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

File already in Hatrac, update with new filename #1009

Merged
merged 5 commits into from
Mar 22, 2024
Merged

Conversation

jrchudy
Copy link
Member

@jrchudy jrchudy commented Mar 20, 2024

This PR intends to modify the fileExists check in hatrac.js for updating the content-disposition for a file that already exists (same md5 and content), except the new file has a different name.

One note about this PR is that a new logging action is being added, "upload/metadata/update". This follows a similar pattern with the existing createUploadJob that sets "upload/create" as the action. I figured "upload/update" wasn't specific enough (since we are only updating metadata, not the whole file) and in this case we are "updating" existing content rather than creating something new.

For documentation, where should this "action" be added? I didn't see our other action for upload in the Action List spreadsheet.

@jrchudy jrchudy requested a review from RFSH March 20, 2024 18:15
@jrchudy jrchudy self-assigned this Mar 20, 2024
@jrchudy jrchudy changed the title File already in Hatrac with new filename File already in Hatrac, update with new filename Mar 20, 2024
js/hatrac.js Outdated Show resolved Hide resolved
@jrchudy
Copy link
Member Author

jrchudy commented Mar 21, 2024

@RFSH I've updated the code to make the change you suggested. It makes sense to update the content-disposition in the fileExists function in hatrac.js since chaise won't act on this any differently. We aren't planning on communicating to the user that a file has metadata updated instead of a file being uploaded in the UI, so there's no need to separate these steps.

One note I should have mentioned before is about the logging. I updated the main body above to comment about the added log action and if we should go with what I proposed or if there is a better phrase to use.

@jrchudy jrchudy requested a review from RFSH March 21, 2024 18:41
js/hatrac.js Outdated

var data = "filename*=UTF-8''" + self.file.name.replace(FILENAME_REGEXP, '_');

if (!contextHeaderParams || !_isObject(contextHeaderParams)) {
Copy link
Member

Choose a reason for hiding this comment

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

If contextHeaderParams wasn't passed to this function, line 417-423 would create it. So this if statement is pointless and this code will always use the same action for both requests. Instead you should assume contextHeaderParams is available here and do

contextHeaderParams.action = 'upload/metadata/update';

So

...

var data = "filename*=UTF-8''" + self.file.name.replace(FILENAME_REGEXP, '_');

contextHeaderParams.action = 'upload/metadata/update';
var config = {
    headers: _generateContextHeader(contextHeaderParams)
};
config.headers['content-type'] = 'text/plain';

...

Copy link
Member

@RFSH RFSH left a comment

Choose a reason for hiding this comment

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

I manually tested uploading on chaise and ran the unit test cases. Everything works as epcted.

The changes overall look good and I just asked for a small change related to how the logging is done.

@jrchudy jrchudy merged commit e2f9033 into master Mar 22, 2024
1 check passed
@jrchudy jrchudy deleted the hatrac-new-filename branch March 22, 2024 20:46
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.

2 participants