-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
Add securetoken test Update tests Fix failed test Change the end time
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.
Looks good to me! Couple questions, but I'd also 👍 as-is
app/helpers/dams_objects_helper.rb
Outdated
file_name = grab_file_name(field_data, cmp_id) | ||
return nil unless file_name | ||
obj_path = obj_id.scan(/.{1,2}/).join('/') | ||
"#{base_url}#{obj_path}/20775-#{obj_id}-#{file_name}".html_safe |
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 is a little nitpicky, but I'm wondering about having 20775 hardcoded here (and I think I saw it in one or two other places too. Should we put this in a config file or a one line method? I think the title for this would be something like ark_naan
making an acronym out of Name Assigning Authority Number
app/helpers/dams_objects_helper.rb
Outdated
# @return string or nil | ||
#--- | ||
|
||
def grab_file_name(field_data, cmp_id) |
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.
is this used for non audio-video files? It seems specifically a look up for them. If so, maybe the name of the method should indicate that?
@mcritchlow - The code has been updated. Could you review them again? Thanks |
LGTM 👍 |
👍 |
Fixes #448 ; refs #448
@ucsdlib/developers - please review