Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix displaying auth scope #38

Merged
merged 3 commits into from
Nov 20, 2018
Merged

Fix displaying auth scope #38

merged 3 commits into from
Nov 20, 2018

Conversation

dennis-zent
Copy link
Contributor

#37

@dennis-zent
Copy link
Contributor Author

hm~ Checking code coverage is odd.
I think codes of that lines are executed.

@danielb2
Copy link
Owner

the coverage reporting tool is accurate. Want to check it again ?

@dennis-zent
Copy link
Contributor Author

Test result in my local mac os is following

$ npm run test

> blipp@3.1.1 test /Users/dennis/github/blipp
> lab -a code -v -t 100 -L

routes
  ✔ 1) print route information (32 ms and 4 assertions)
  ✔ 2) gets route information (9 ms and 3 assertions)
required: tester1 forbidden: tester3 selection: tester2
required: tester1 forbidden: tester3 selection: tester2
  ✔ 3) gets route information with auth (8 ms and 5 assertions)
required: tester1 forbidden: tester3 selection: tester2
required: tester1 forbidden: tester3 selection: tester2
  ✔ 4) gets route information with scope (7 ms and 5 assertions)
required: tester1 forbidden: tester3 selection: tester2
required: tester1 forbidden: tester3 selection: tester2
  ✔ 5) gets route information with default (11 ms and 4 assertions)
  ✔ 6) fails with invalid options (7 ms and 2 assertions)


6 tests complete
Test duration: 81 ms
Assertions count: 23 (verbosity: 3.83)
No global variable leaks detected
Coverage: 95.95% (6/148)
lib/index.js missing coverage on line(s): 126, 127, 128, 129, 130, 131
Code coverage below threshold: 95.95 < 100
Linting results: No issues

I inserted debug message like following

125:        if (route.settings.auth && route.settings.auth.access) {
126:            const required = route.settings.auth.access[0].scope.required ?
127:                `required: ${route.settings.auth.access[0].scope.required.toString()} ` : '';
128:            const forbidden = route.settings.auth.access[0].scope.forbidden ?
129:                `forbidden: ${route.settings.auth.access[0].scope.forbidden.toString()} ` : '';
130:            const selection = route.settings.auth.access[0].scope.selection ?
131:                `selection: ${route.settings.auth.access[0].scope.selection.toString()}` : '';

            authScope = `${required}${forbidden}${selection}`;
            console.log(authScope);
        }

I think those lines are executed.

@danielb2
Copy link
Owner

While the lines may all be executed, are all the variations of the conditionals executed ?

Those are all ternaries.

I recommend you use Hoek.reach to make this simpler.

Hoek.reach(route, 'settings.auth.access.0.scope.selection', { default: '' });

For example

@dennis-zent
Copy link
Contributor Author

Ok. I understand.

I will add test cases and rewrite my codes. :)

@danielb2
Copy link
Owner

Thank you @Dennis-Emmental

@treyhuffine
Copy link

@danielb2 has this change been published? I'm still seeing the old code in v3.1.1 on npm and getting the error.

Thanks for the maintaining the package, it's great.

@danielb2
Copy link
Owner

I think I had something else in mind to release at the same time, but obviously that hasn't happened. I'lll do it now :) thank you for the reminder

@danielb2
Copy link
Owner

ok, done! sorry for the delay

@treyhuffine
Copy link

Thanks! Works great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants