-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Just a few comments where we could possibly remove some conditional branching, and a question about test titles. Overall LGTM!
@@ -8,6 +8,16 @@ | |||
</div> | |||
</div> | |||
<% end %> | |||
<% if !can? :update, @document then %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use cannot
here instead of !can?
@@ -8,6 +8,16 @@ | |||
</div> | |||
</div> | |||
<% end %> | |||
<% if !can? :update, @document then %> | |||
<% accessNotice = grab_access_text(@document) %> | |||
<% unless accessNotice.nil? %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be flipped as if accessNotice
@@ -30,11 +30,19 @@ | |||
<div class="simple-object"></div> | |||
<% end %> | |||
<%# END RESTRICTED NOTICE %> | |||
|
|||
|
|||
<% if !can? :update, @document then %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use cannot
here instead of !can?
|
||
<% if !can? :update, @document then %> | ||
<% accessNotice = grab_access_text(@document) %> | ||
<% unless accessNotice.nil? %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be flipped as if accessNotice
lib/dams/controller_helper.rb
Outdated
|
||
def metadata_display?(data) | ||
result = false | ||
unless data.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be if data
or you could drop the conditional and do a return unless data
app/helpers/dams_objects_helper.rb
Outdated
result = nil | ||
access_group = document['read_access_group_ssim'] # "public" > "local" > "dams-curator" == "dams-rci" == default | ||
data = document['otherRights_tesim'] | ||
unless data.nil? && access_group.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could probably do a return result unless data && access_group
here if you want do drop the conditional
app/helpers/dams_objects_helper.rb
Outdated
def get_attribution_note(data) | ||
result = 'Content not available. Access may granted for research purposes at the discretion of the UC San Diego Library. For more information please contact the ' | ||
program_email = { 'Digital Library Development Program' => 'dlp@ucsd.edu', 'Special Collections & Archives' => 'spcoll@ucsd.edu', 'Research Data Curation Program' => 'research-data-curation@ucsd.edu'} | ||
unless data.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could probably do a return result unless data
here and drop the conditional
spec/features/dams_object_spec.rb
Outdated
end | ||
|
||
scenario 'should see Restricted View access control information' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the scenario titles perhaps refer to the kind of user and what they can or can't see? It might help if tests fail in the future to see what kind of access control/user types aren't seeing the right things.
Update access note function Fix hound violation Fix hound issue Update test Fix build error Comment out failed test Fix build error Fix error Update circleci config Fix build error Remove debug message Update qa deploy config Update restricted message and tests Fix access control tests issue Update config file Update restricted note message Update access rule for restricted note Add more test Add program email to restricted note Update test Update Test and Restricted Text function Skip the random controller test
eacdc3a
to
9be9cc2
Compare
@mcritchlow - I updated the code with all the changes you mentioned above. I also deployed the changes to qa and confirmed they worked as expected. Could you review them again? Thanks |
👍 |
1 similar comment
👍 |
Fixes #413 ; refs #413
Add restricted view banner note for metadata-only object view.
@ucsdlib/developers - please review