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
Weblab filesapi rename case only #15287
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.
LG, just a few small nits.
not_found if src_entry.nil? | ||
new_entry_json = src_entry.to_json | ||
else | ||
new_entry_json = copy_file('files', encrypted_channel_id, URI.encode(unescaped_filename_downcased), URI.encode(unescaped_src_filename_downcased)) |
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.
nit: This line is a too long, please break it up as
new_entry_json = copy_file(
'files',
encrypted_channel_id,
URI.encode(unescaped_filename_downcased),
URI.encode(unescaped_src_filename_downcased)
)
@@ -531,7 +541,7 @@ def files_put_file(encrypted_channel_id, filename, body) | |||
end | |||
|
|||
# delete a file if requested (same as src file in a rename operation) | |||
bucket.delete(encrypted_channel_id, params['delete'].downcase) if params['delete'] | |||
bucket.delete(encrypted_channel_id, URI.encode(unescaped_delete_filename_downcased)) if deleting_separate_file |
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.
nit: Another long line.
if deleting_separate_file
bucket.delete(encrypted_channel_id, URI.encode(unescaped_delete_filename_downcased))
end
get "v3/files/#{@channel_id}" | ||
response_after_rename = JSON.parse(last_response.body) | ||
# There should be only one file with the new filename, category, and size matching our expectations | ||
expected_image_info_after_rename = {'filename' => filename2, 'category' => 'image', 'size' => image_body.length} |
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 clearer as expected_image_info_after_rename = expected_image_info.merge('filename' => filename2)
?
assert_fileinfo_equal(expected_image_info_after_rename, file_infos_after_rename[0]) | ||
|
||
# The manifest version (filesVersionId) should be different, but the file version should be the same | ||
assert response_before_rename['filesVersionId'] != response_after_rename['filesVersionId'] |
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.
Perhaps use refute_equal
here, as doing so may provide a more helpful error message should the test fail here.
@ashercodeorg I'll make those changes locally and include in my next PR. Thanks for the feedback. |
foo.html
toFoo.html
). Cleaned up variable names infiles_put_file
relating to the 3 filenames in play (the new file, the source file, and the file to be deleted). It is now more clear which are unescaped and/or downcased.case_only_rename