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

Adds count class level permission #3814

Merged
merged 4 commits into from
May 15, 2017
Merged

Adds count class level permission #3814

merged 4 commits into from
May 15, 2017

Conversation

flovilmart
Copy link
Contributor

@flovilmart flovilmart commented May 14, 2017

Partial resolution for #3813

@flovilmart flovilmart requested a review from acinader May 14, 2017 02:50
@codecov
Copy link

codecov bot commented May 14, 2017

Codecov Report

Merging #3814 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3814      +/-   ##
==========================================
+ Coverage   90.14%   90.17%   +0.03%     
==========================================
  Files         114      114              
  Lines        7526     7529       +3     
==========================================
+ Hits         6784     6789       +5     
+ Misses        742      740       -2
Impacted Files Coverage Δ
src/Controllers/DatabaseController.js 94.36% <100%> (+0.01%) ⬆️
src/triggers.js 87.76% <100%> (+0.13%) ⬆️
src/Controllers/SchemaController.js 97.04% <100%> (ø) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.31% <0%> (ø) ⬆️
src/Adapters/Cache/InMemoryCache.js 100% <0%> (+7.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d71683a...930bc03. Read the comment docs.

Copy link
Contributor

@acinader acinader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. good for me to see tool. can you give me some clarity on the unit test description question i made?

});
});

it('should report count if passed', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test description is the same as above. maybe should be 'should report find if passed', but i'm confused what you're testing and why. what does it mean "to report" in this context?

i just see that the result is zero as expected and your hook is called for both find and count.

return schema.setPermissions('Stuff', {
'create': {'*': true},
'find': {'*': true},
'count': count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm assuming that count = {} is roughly equivelent to count = { '*': false } ?

Copy link
Contributor Author

@flovilmart flovilmart May 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but *: false is not a valid CLP, {} means lockdown, no one can execute a count

.then(schema => {
var count = {};
return schema.setPermissions('Stuff', {
'create': {'*': true},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no quotes around object members

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was to match the style of the rest of the file, we'll update those at a later time (i.e. test linting)

src/triggers.js Outdated
var request = {
triggerName: triggerType,
query: query,
master: false,
count: count,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi, you could just have count instead of count: count.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was to match the style, I'll update :)

@flovilmart
Copy link
Contributor Author

@acinader pushed some nits

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.

None yet

2 participants