-
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
Show historical OldDot Report Actions in Plain text #38013
Show historical OldDot Report Actions in Plain text #38013
Conversation
757cf77
to
a85e541
Compare
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@twisterdotcom Kindly help to review if all oldDot actions needed are included, thanks! You can just comment here if there're other types need to add. I'll help update them then. |
@eh2077 - I checked yesterday when chatting this through with @dylanexpensify - the only one missing was the EXPORTEDTOCSV one which I pushed above. Other than that, we're good to go! Edit, ah wait you did it, great! No, nothing left to do here except review and get it merged. |
Oh no, perf-tests is failing. Try to re-run it @eh2077 |
Reviewer Checklist
Screenshots/Videos |
Re-running it 👍 |
@hoangzinh Good catch. Yes, in this test case, the App/src/libs/ReportActionsUtils.ts Lines 912 to 914 in 4e599b2
@twisterdotcom Do we need to show those system messages from oldDot actions in LHN? If we want to show them, then I think we need to sort out them case by case because many of them may need corresponding Spanish translations. Or I guess we can just omit showing them in LHN because they're legacy actions which won't be added to newDot? |
@eh2077 - we can omit them from the LHN for now. We have plans to slowly migrate these all to native newDot actions - this is more a temporary fix to resolve confusion before we build them into newDot with Spanish translations and better wording to fit the chat flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@Beamanator looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Uhhh the checklist point failed even though it was filled out here - #38013 (comment) |
✋ 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/Beamanator in version: 1.4.54-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.54-4 🚀
|
DELETEDACCOUNT: 'DELETEDACCOUNT', // OldDot Action | ||
DONATION: 'DONATION', // OldDot Action | ||
EXPORTEDTOCSV: 'EXPORTEDTOCSV', // OldDot Action | ||
EXPORTEDTOINTEGRATION: 'EXPORTEDTOINTEGRATION', // OldDot Action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed the value for this one is off. It should just be EXPORTINTEGRATION
(src). @Beamanator can you update that? We're going to need it for the QBO doc I'm working on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @arosiclair ! fixed here: #42242
Details
This PR incorporates commits from #37022 and works on them.
Fixed Issues
$ #36023
$ #34968
$ #34783
$ #32602
PROPOSAL:
Tests
Pre-testing steps
i. Enable scheduled submit with a frequency of Instant (this should happen by default with Expensify accounts, or accounts on paidPolicyInstantSubmit beta)
ii. Make sure the approval mode is "Submit & Approve"
iii. Invite an additional user to the policy to play the member role in these testing flows
Test steps
In OldDot: As submitter, make sure ^ new workspace is your "Active" / default workspace
In OldDot: As submitter, create a report and add an expense
In OldDot: As Admin, approve the report - then Unapprove the report
In NewDot: As both accounts, view the expense report & verify you can see both of these report actions:
i. final approved this report
ii. unapproved this report
Verify that
unapproved
this report report action appear.Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
0-web.mp4
MacOS: Desktop