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

Cache file upload original names #35429

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

MartinRiese
Copy link
Contributor

@MartinRiese MartinRiese commented Nov 25, 2024

Product Description

When deleting a repeat group we need to rebuild all the following groups because the IDs are reassigned by formplayer. Unfortunately FP does not keep track of the original filename automatically and only sends the uuid based generated filename for the upload in the delete-repeat response. To be able to still display the original filename we store it in a map with the generated name as the key. That way we can figure out what it is when we rebuild that part of the UI.

Technical Summary

https://dimagi.atlassian.net/browse/USH-5078

Feature Flag

Safety Assurance

tested locally and on staging

Safety story

Automated test coverage

corehq/apps/cloudcare/static/cloudcare/js/form_entry/spec/form_ui_spec.js

QA Plan

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Nov 25, 2024
@MartinRiese MartinRiese force-pushed the riese/filename_broadcast branch 2 times, most recently from a978dd1 to f8e93c5 Compare November 25, 2024 17:51
When deleting a repeat group we need to rebuild all the following groups
because the IDs are reassigned by formplayer. Unfortunatly FP does not
keep track of the original filename automatically and only sends the
uuid based generated filename for the upload in the delete-repeat
response. To be able to still display the original filename we store it
in a map with the generated name as the key. That way we can figure out
what it is when we rebuild that part of the UI.
@MartinRiese MartinRiese force-pushed the riese/filename_broadcast branch from f8e93c5 to 751629b Compare November 25, 2024 18:08
@MartinRiese MartinRiese marked this pull request as ready for review November 26, 2024 15:47
Copy link
Contributor

@Robert-Costello Robert-Costello left a comment

Choose a reason for hiding this comment

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

This looks good to me. Not blocking, but is it possible to add a test/assert that checks the filename is cached and/or reassigned after a repeat is deleted?

self.fileNameDisplay(fixedNewValue);
self.answer(fixedNewValue);
} else if (cachedFilename) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what the code above is doing? What situation is newValue an empty string vs constants.NO_ANSWER (null)?

        if (newValue === "" && self.question.filename) {
            self.question.hasAnswered = true;
            self.fileNameDisplay(self.question.filename());

It looks like Minha added this previously here to solve a similar issue with the file question being rebuilt for question tiles. I'm not understanding why that doesn't solve the same issue here.

@@ -938,7 +938,8 @@ hqDefine("cloudcare/js/form_entry/entries", [
FileEntry.prototype = Object.create(EntrySingleAnswer.prototype);
FileEntry.prototype.constructor = EntrySingleAnswer;
FileEntry.prototype.onPreProcess = function (newValue) {
var self = this;
const self = this;
const cachedFilename = self.question.form().fileNameCache[self.answer()];
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the distinction between self.answer() and self.question.answer()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is none. self.answer just references to self. question.answer

commitAdd condition I missed in last commitAdd condition I missed in
last commitAdd condition I missed in last commitAdd condition I missed
in last commitAdd condition I missed in last commitAdd condition I
missed in last commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants