-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2023-08-16][$1000] Attachment ValidationModal texts are not translated to Spanish #22665
Comments
Triggered auto assignment to @adelekennedy ( |
Bug0 Triage Checklist (Main S/O)
|
Proposalby @hungvu193 Please re-state the problem that we are trying to solve in this issue.Attachment Validation Modal's texts are not translated to Spanish What is the root cause of that problem?We're setting text as title and prompt for the ConfirmModal in here: App/src/components/AttachmentModal.js Lines 184 to 207 in cc49692
App/src/components/AttachmentModal.js Lines 352 to 360 in cc49692
That's why when we changed the language, only Close button text was translated. What changes do you think we should make in order to solve the problem?We should get rid of text, since it's required re-render to make it translated. We should use translate key instead. if (_.contains(CONST.API_ATTACHMENT_VALIDATIONS.UNALLOWED_EXTENSIONS, fileExtension.toLowerCase())) {
const invalidReason = 'attachmentPicker.notAllowedExtension';
setAttachmentInvalidReasonTitle('attachmentPicker.wrongFileType');
setAttachmentInvalidReason(invalidReason);
setIsAttachmentInvalid(true);
return false;
}
if (lodashGet(_file, 'size', 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) {
setAttachmentInvalidReasonTitle('attachmentPicker.attachmentTooLarge');
setAttachmentInvalidReason('attachmentPicker.sizeExceeded');
setIsAttachmentInvalid(true);
return false;
}
if (lodashGet(_file, 'size', 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
setAttachmentInvalidReasonTitle('attachmentPicker.attachmentTooSmall');
setAttachmentInvalidReason('attachmentPicker.sizeNotMet');
setIsAttachmentInvalid(true);
return false;
} And finally update the title and prompt to use <ConfirmModal
// we should use safe check here to prevent crash because of missing translate key
title={!!attachmentInvalidReasonTitle ? props.translate(attachmentInvalidReasonTitle) : ''}
onConfirm={closeConfirmModal}
onCancel={closeConfirmModal}
isVisible={isAttachmentInvalid}
prompt={!!attachmentInvalidReason ? props.translate(attachmentInvalidReason) : ''}
confirmText={props.translate('common.close')}
shouldShowCancelButton={false}
/> What alternative solutions did you explore? (Optional)N/A ResultScreen.Recording.2023-07-10.at.23.28.54.mov |
posted in Slack I don't see this as a user problem, it's edge case behavior |
@adelekennedy I think it's actually bug since we set text for the modal instead of "translateKey", we should avoid using that. |
hmm @hungvu193 it sounds like this is something you think we should fix overall that's leading to related bugs (these are all very edge case behaviors - but I can see an argument for opening one issue to fix them all) |
@adelekennedy Yep, we should fix them all, I found into our project and there're 2 places (one in this issue you mentioned) and one with TaskAssigneePage. |
Updated my proposal to address issue from ProposalPlease re-state the problem that we are trying to solve in this issue.Attachment Validation Modal's texts are not translated to Spanish What is the root cause of that problem?We're setting text as title and prompt for the ConfirmModal in here: App/src/components/AttachmentModal.js Lines 184 to 207 in cc49692
App/src/components/AttachmentModal.js Lines 352 to 360 in cc49692
That's why when we changed the language, only Close button text was translated. What changes do you think we should make in order to solve the problem?We should get rid of text, since it's required re-render to make it translated. We should use translate key instead. if (_.contains(CONST.API_ATTACHMENT_VALIDATIONS.UNALLOWED_EXTENSIONS, fileExtension.toLowerCase())) {
const invalidReason = 'attachmentPicker.notAllowedExtension';
setAttachmentInvalidReasonTitle('attachmentPicker.wrongFileType');
setAttachmentInvalidReason(invalidReason);
setIsAttachmentInvalid(true);
return false;
}
if (lodashGet(_file, 'size', 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) {
setAttachmentInvalidReasonTitle('attachmentPicker.attachmentTooLarge');
setAttachmentInvalidReason('attachmentPicker.sizeExceeded');
setIsAttachmentInvalid(true);
return false;
}
if (lodashGet(_file, 'size', 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
setAttachmentInvalidReasonTitle('attachmentPicker.attachmentTooSmall');
setAttachmentInvalidReason('attachmentPicker.sizeNotMet');
setIsAttachmentInvalid(true);
return false;
} And finally update the title and prompt to use <ConfirmModal
// we should use safe check here to prevent crash because of missing translate key
title={!!attachmentInvalidReasonTitle ? props.translate(attachmentInvalidReasonTitle) : ''}
onConfirm={closeConfirmModal}
onCancel={closeConfirmModal}
isVisible={isAttachmentInvalid}
prompt={!!attachmentInvalidReason ? props.translate(attachmentInvalidReason) : ''}
confirmText={props.translate('common.close')}
shouldShowCancelButton={false}
/> I also found out that we are using it in our <ConfirmModal
title={this.state.errorModalTitle ? this.props.translate(this.state.errorModalTitle) : ''}
onConfirm={this.hideErrorModal}
onCancel={this.hideErrorModal}
isVisible={this.state.isErrorModalVisible}
prompt={this.state.errorModalPrompt ? this.props.translate(this.state.errorModalPrompt) : ''}
confirmText={this.props.translate('common.close')}
shouldShowCancelButton={false}
/> Overall, they had same RCA, so we can address any issue related to this one in here. What alternative solutions did you explore? (Optional)N/A ResultScreen.Recording.2023-07-10.at.23.28.54.mov |
Okay! Let's keep this open and close the issues that are coming back to the same root issue |
Job added to Upwork: https://www.upwork.com/jobs/~01af42706e42700678 |
Current assignee @adelekennedy is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
not overdue melvin |
@hungvu193 Any Idea where was this broken? |
AFAIK, there're AvatarWithImagePicker, AttachmentModal and NewTaskPage. |
📣 @hungvu193 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app! |
@adelekennedy That makes sense, let's hold this for #23754 |
I put a hold on this one pending the outcome of #23754. I've also made it a weekly for now. |
Thank you @deetergp! |
#23754 is off hold, we can resume the work here. |
Cool. I'll start a PR later today |
🎯 ⚡️ Woah @Santhosh-Sellavel / @hungvu193, great job pushing this forwards! ⚡️ The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉
On to the next one 🚀 |
@adelekennedy or @deetergp remove the Hold from title thanks! |
@Santhosh-Sellavel Done! |
This has been deployed to production 2 days ago. Please update labels @adelekennedy |
huh - annoying that it isn't updating automatically. |
Payouts due:Contributor: $1000 for issue fix, + reporting bonus ($250) @hungvu193 (paid via Upwork) Eligible for 50% #urgency bonus? Y Upwork job is here. |
@Santhosh-Sellavel will you post the checklist? |
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Requested on ND |
Reviewed the details for @Santhosh-Sellavel. $1,500 approved for payment via NewDot based on BZ summary. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
Modal text (title, prompt) should be translated to Spanish.
Actual Result:
Modal texts (title, prompt) aren't translated to Spanish.
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.39-5
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screen.Recording.2023-07-10.at.23.32.37.mov
Recording.1241.mp4
Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689006789754909
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: