[No QA] ReportNextStep: New format#71855
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
We don't need to wait for translation as this won't be displayed to users yet. I have just merged main, most of the failing tests do no look related except one which I fixed. |
|
@dangrous Can you take a look? Thanks |
dangrous
left a comment
There was a problem hiding this comment.
okay this looks great! just a couple of questions
| case CONST.NEXT_STEP.ACTOR_TYPE.CURRENT_USER: | ||
| return `Waiting for <strong>your</strong> expenses to automatically submit${formattedETA}.`; | ||
| case CONST.NEXT_STEP.ACTOR_TYPE.OTHER_USER: | ||
| return `Waiting for <strong>${actor}'s</strong> expenses to automatically submit${formattedETA}.`; | ||
| case CONST.NEXT_STEP.ACTOR_TYPE.UNSPECIFIED_ADMIN: | ||
| return `Waiting for an admin's expenses to automatically submit${formattedETA}.`; |
There was a problem hiding this comment.
@jamesdeanexpensify since you've been working on these lately - is this the message we want in ND? Or should it just be Waiting for expenses... instead of your/actor's
| }, | ||
| }); | ||
|
|
||
| function buildNextStepMessage(nextStep: ReportNextStep, translate: LocaleContextProps['translate']): string { |
There was a problem hiding this comment.
is this essentially the same logic as the existing optimistic next step code, just with the new format?
There was a problem hiding this comment.
This is not the optimistic next step builder, it's buildOptimisticNextStep and yes that one is basically same as buildNextStepNew but for new the format.
buildNextStepMessage is the function that generates the message given the report next step object, it's kinda the equivalent of parseMessage but for the new format.
There was a problem hiding this comment.
oh got it, thanks that's helpful!
|
many many conflicts now (sorry!) but then I think we're good to go! |
|
Resolved conflicts |
dangrous
left a comment
There was a problem hiding this comment.
one more conflict, looks good thoguh!
|
Merged main |
|
sigh, now an (unrelated?) typecheck error. Can you double check and merge main one more time? Sorry this is dragging so much... |
|
Yeah I saw that error in other places. Should be fixed now |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No review require from the product team. Unsubscribing.
dangrous
left a comment
There was a problem hiding this comment.
Appreciate all the back and forth here, let's do it!
|
✋ 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/dangrous in version: 9.2.58-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.2.58-3 🚀
|
Explanation of Change
The next step is now part of the report's data (
report.nextStep). This PR adds support to that new format and optimistically update nextStep. This is basically a preparation and shouldn't impact existing behavior. Once we start sending nextStep from backend, we'd need to updateMoneyReportHeaderandMoneyReportHeaderStatusBarto actually make use of that.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/528713
PROPOSAL:
Tests
n/a
Offline tests
n/a
QA Steps
n/a
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop