Update how DEW submit actions are displayed#83517
Conversation
Coming from `npx ts-node ./scripts/generateTranslations.ts --compare-ref=main` since GH action wasn't working
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| const wasSubmittedViaHarvesting = getOriginalMessage(action)?.harvesting ?? false; | ||
|
|
||
| let failedSubmitReason = getOriginalMessage(action)?.message ?? translate('iou.error.genericCreateFailureMessage'); | ||
| failedSubmitReason = wasSubmittedViaHarvesting ? translate('iou.failedToAutoSubmitViaDEW', failedSubmitReason) : translate('iou.failedToSubmitViaDEW', failedSubmitReason); |
There was a problem hiding this comment.
NAB, but I prefer to not mutating variable, so we can rename the first one as const message and keep the 2nd one name.
| failedSubmitReason = wasSubmittedViaHarvesting ? translate('iou.failedToAutoSubmitViaDEW', failedSubmitReason) : translate('iou.failedToSubmitViaDEW', failedSubmitReason); | |
| const message = getOriginalMessage(action)?.message ?? translate('iou.error.genericCreateFailureMessage'); | |
| const failedSubmitReason = wasSubmittedViaHarvesting ? translate('iou.failedToAutoSubmitViaDEW', message) : translate('iou.failedToSubmitViaDEW', message); |
|
|
||
| children = ( | ||
| <ReportActionItemBasicMessage> | ||
| <RenderHTML html={`<comment><muted-text>${failedSubmitReason}</muted-text></comment>`} /> |
There was a problem hiding this comment.
Since we want to render this as HTML, we can move the logic to getReportActionMessageFragments and remove the rendering here.
App/src/libs/ReportActionsUtils.ts
Lines 2252 to 2309 in 9adcde0
if (isDynamicExternalWorkflowSubmitFailedAction(action)) {
const originalMessage = getOriginalMessage(action);
const wasSubmittedViaHarvesting = originalMessage?.harvesting ?? false;
const message = originalMessage?.message ?? translate('iou.error.genericCreateFailureMessage');
const failedSubmitReason = wasSubmittedViaHarvesting ? translate('iou.failedToAutoSubmitViaDEW', message) : translate('iou.failedToSubmitViaDEW', message);
return [{text: message, html: `<muted-text>${failedSubmitReason}</muted-text>`, type: 'COMMENT'}];
}|
|
||
| children = ( | ||
| <ReportActionItemBasicMessage> | ||
| <RenderHTML html={`<comment><muted-text>${failedApproveReason}</muted-text></comment>`} /> |
|
For the test step,
(recording for the checklist later) web.mp4 |
|
Updated, also added some tests.
Yeah, I don't think there's an easy way to test this part because harvesting can't be manually triggered, as far as I know (well, at least on purpose). The options for local testing are:
I'll do internal QA for that part
👍 I'll update the steps |
…o-submit-error-from-concierge
| @@ -1,5 +1,5 @@ | |||
| import Parser from '@libs/Parser'; | |||
| import getClipboardText from '@libs/Clipboard/getClipboardText'; | |||
| import Parser from '@libs/Parser'; | |||
There was a problem hiding this comment.
This change is unrelated to the PR, but the prettier step was failing from CI, I had to pull main and run npm run prettier to fix it.
|
(@bernhardoj will do the C+ review on this because he already has context btw) |
src/libs/ReportActionsUtils.ts
Outdated
| const wasSubmittedViaHarvesting = originalMessage?.harvesting ?? false; | ||
| const message = originalMessage?.message ?? translate('iou.error.genericCreateFailureMessage'); | ||
| const failedSubmitReason = wasSubmittedViaHarvesting ? translate('iou.failedToAutoSubmitViaDEW', message) : translate('iou.failedToSubmitViaDEW', message); | ||
| return [{text: message, html: `<muted-text>${failedSubmitReason}</muted-text>`, type: 'COMMENT'}]; |
There was a problem hiding this comment.
My bad, the text should be failedSubmitReason
There was a problem hiding this comment.
Ah I see, makes sense. In the end it didn't change anything because we only render the html text? I'll still edit though.
There was a problem hiding this comment.
Yeah, it didn't change anything though.
src/libs/ReportActionsUtils.ts
Outdated
| const wasAutoApproveAction = originalMessage?.automaticAction ?? false; | ||
| const message = originalMessage?.message ?? translate('iou.error.genericCreateFailureMessage'); | ||
| const failedApproveReason = wasAutoApproveAction ? translate('iou.failedToAutoApproveViaDEW', message) : translate('iou.failedToApproveViaDEW', message); | ||
| return [{text: message, html: `<muted-text>${failedApproveReason}</muted-text>`, type: 'COMMENT'}]; |
|
Ok, updated! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mp4 |
src/languages/es.ts
Outdated
| queuedToSubmitViaDEW: 'en cola para enviar a través del flujo de aprobación personalizado', | ||
| failedToAutoSubmitViaDEW: (reason: string) => `no ha podido enviar este informe mediante <a href="${CONST.SELECT_WORKFLOWS_HELP_URL}">retrasar envíos</a>. ${reason}`, | ||
| failedToSubmitViaDEW: (reason: string) => `no ha podido enviar este informe. ${reason}`, | ||
| failedToAutoApproveViaDEW: (reason: string) => `no ha podido aprobar mediante <a href="${CONST.CONFIGURE_EXPENSE_REPORT_RULES_HELP_URL}">workspace rules</a>. ${reason}`, |
There was a problem hiding this comment.
Before approving, have we confirmed the translation? Also, is it expected not translating the "workspace rules"?
There was a problem hiding this comment.
Oh good catch for workspace rules, that's a bad copy/paste. I'm confirming the rest of the translation internally but let's not hold the PR on that, it's a minor thing we can adjust later if needed.
2fd3758 to
1562f06
Compare
|
Updated again. |
|
🎯 @bernhardoj, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #83681. |
Conflicts:
src/languages/de.ts
src/languages/fr.ts
src/languages/it.ts
src/languages/ja.ts
src/languages/nl.ts
src/languages/pl.ts
src/languages/pt-BR.ts
src/languages/zh-hans.ts
|
🚧 @mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.3.28-0 🚀
|
|
This PR is failing because of issue ##84043 The issue is reproducible in: Web |
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.3.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.30-3 🚀
|
Explanation of Change
Update how submit errors are shown when a policy uses DEW, to better match the original design.
failed to submit/approveprefixFixed Issues
$ https://github.com/Expensify/Expensify/issues/603317
Offline tests
N/A
QA Steps
Important
Sign in to one of following accounts to run QA:
If you need to use a different account, tag @francoisl in the QA slack channel for help
Same as test steps below
Tests
Prerequisite: use an account with a workspace that has a test DEW implementation enabled
Setup:
failed to submit the report. Add at least one approver as an expense descriptionand that the "actor" of the message is yourselfAUTOAPPROVE_ERRORreport routed to XYZ due to custom approval workflowcoming from Concierge is added to the reportAPPROVE_ERRORfailed to approve. This report cannot be approvedcoming from the admin (not Concierge) was added to the reportPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
MacOS: Chrome / Safari