MMT-855: Adds ability to choose 'No Access to Collections' for Collection Permissions #1
Conversation
| # is ~40, so 100 should be more than sufficient | ||
| filters = { | ||
| 'provider' => current_user.provider_id, | ||
| 'page_size' => 100 |
There was a problem hiding this comment.
We shouldn't make assumptions about min and max sizes of lists. If you would like to default page number to 100 that's fine (though I'd prefer we stick to a more app wide default for this) but we should be paging through these results to ensure nothing is missed.
| let(:perm_concept) { 'ACL4444-CMR' } | ||
|
|
||
| before do | ||
| permission_response = Cmr::Response.new(Faraday::Response.new(status: 200, body: JSON.parse(File.read('spec/fixtures/collection_permissions/permission_show_w_no_collections_access.json')))) |
There was a problem hiding this comment.
Is this something we can hit CMR for instead of mocking? When it comes to CMR I think we should mock as little as possible given the pace at which they make changes and the impact it has on us.
|
|
||
| def construct_request_object(provider) | ||
| collection_applicable = false | ||
| if params[:collection_options] == 'all-collections' || params[:collection_options] == 'selected-ids-collections' |
There was a problem hiding this comment.
You are using a different method of setting a value using an if statement below on line 505 -- I prefer the method below but regardless please be consistent.
| else | ||
| 'all-collections' | ||
| end | ||
| else # catalog_item_identity['collection_applicable'] == false |
There was a problem hiding this comment.
Not sure what this comment is for, if it's not relevant please remove it.
There was a problem hiding this comment.
The comment is just noting what the condition is to get to the else statement, but I can remove it.
| # and at least one group with the extra permissions | ||
|
|
||
| before do | ||
| collections_response = Cmr::Response.new(Faraday::Response.new(status: 200, body: JSON.parse(File.read('spec/fixtures/cmr_search.json')))) |
There was a problem hiding this comment.
Is this something we can hit CMR for instead of mocking? When it comes to CMR I think we should mock as little as possible given the pace at which they make changes and the impact it has on us.
|
|
||
| # PUMP can also add group permissions where the group has empty permissions so we should test that as well | ||
| context 'when the collection permission has create, update, and delete permissions as well as groups with empty permissions' do | ||
| let(:perm_concept) { 'ACL3333-CMR' } |
There was a problem hiding this comment.
Minor and picky but this abbreviation is misleading, suggest using full word.
| @collection_access_value.each do |key, val| | ||
| if val.blank? | ||
| @collection_access_value.delete(key) | ||
| elsif val == 'true' |
There was a problem hiding this comment.
Is this always a string or should we be wrapping this in a boolean check?
There was a problem hiding this comment.
I am running into this issue: http://stackoverflow.com/questions/1711547/how-to-check-if-a-param-is-true-or-false. Is there a better way to handle it?
abbottry
changed the title
…s to create ACL objects that can be collection_applicable: false
…Change tests for collection permissions to hit cmr instead of using mocks
* Removed bourbon prefix features in set #1 (transform, transition, linear_gradient, user-select). * Removed use of bourbon's hidpi prefix feature. * Updated Gemfile using latest capybara. * Revert schema. * Fix background-* properties for linear-gradient replacements.
Adds ability to choose 'No Access to Collections' for Collection Permissions