-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
fix: fix lhn previews #31061
fix: fix lhn previews #31061
Conversation
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.
Left minor comments.
@mananjadhav 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] |
@mananjadhav this is ready for a review, let us know if you need anything |
@mountiny Thanks I'll have this reviewed. For now, I am going through all the responses by @trjExpensify again, but can we get a confirmation on the QA cases? Do we have any other cases to cover? |
I think the QA cases are good and how it should look like you can see in the latest comments in the issue |
Code changes are fine. @koko57 Can you please merge with the latest main? and I'll start with the testing. |
Also @koko57 can we add the expected results in the Tests section. I can see we end every test with |
@mananjadhav all the results are in the MacOS Chrome / Safari but I can copy paste it higher (I didn't want to make the checklist so long) |
@mananjadhav merged with main 🙂 |
@mananjadhav mind getting this across the line today?? |
Still QAing. Will need a day to work on this. Thanks for the patience. |
@koko57 last merge conflict 🥺 |
@mananjadhav fixed! |
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.
@koko57 Just couple of styling comments
src/libs/OptionsListUtils.js
Outdated
* @param {Boolean} [isPreviewMessageForParentChatReport] | ||
* @returns {String} | ||
*/ | ||
|
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.
src/libs/OptionsListUtils.js
Outdated
*/ | ||
|
||
function getReportPreviewMessageForOptionList(report, reportAction, isPreviewMessageForParentChatReport = false) { | ||
// for the request action preview we want to show the requestor instead of the user who owes the money |
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.
// for the request action preview we want to show the requestor instead of the user who owes the money | |
// For the request action preview we want to show the requestor instead of the user who owes the money |
src/libs/ReportUtils.js
Outdated
* @param {Object} [policy] | ||
* @returns {String} | ||
*/ | ||
|
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.
src/libs/ReportUtils.js
Outdated
* @param {Boolean} [shouldShowWorkspaceName] | ||
* @param {Boolean} [shouldUseShortForm] | ||
* @param {Object} [policy] | ||
* @returns {String} |
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.
* @returns {String} | |
* @returns {String} |
src/libs/ReportUtils.js
Outdated
* @param {Number} actorID | ||
* @param {Boolean} [shouldShowWorkspaceName] | ||
* @param {Boolean} [shouldUseShortForm] | ||
* @param {Object} [policy] |
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.
* @param {Object} [policy] | |
* @param {Object|undefined} [policy] |
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.
Thank you for patience and getting through this one @koko57 🚀
And thanks for testing thoroughly @mananjadhav |
✋ 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/mountiny in version: 1.4.3-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.3-0 🚀
|
* @param {Boolean} [isPreviewMessageForParentChatReport] | ||
* @returns {String} | ||
*/ | ||
function getReportPreviewMessageForOptionList(report, reportAction, isPreviewMessageForParentChatReport = false) { |
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.
@luacmartins I think we can just set default value here:
function getReportPreviewMessageForOptionList(report, reportAction, isPreviewMessageForParentChatReport = false) { | |
function getReportPreviewMessageForOptionList(report, reportAction = {}, isPreviewMessageForParentChatReport = false) { |
This was old code: https://github.com/Expensify/App/pull/31061/files#diff-65c096044d5f69b35bcdec14c99c4fda4580759df9b1a7c36650d58eea276f1dR1861
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.
that will prevent the crash, yes, but why don't we have any reportActions for the given report? When I load the given report I do see reportActions
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.
it may be that the reportActions are only missing for a brief moment while loading/updating onyx
We have another blocker coming from this PR - #31844 so I'm gonna revert it and we can work on a fix without blocking the deploy. |
Another potential blocker - #31833 |
This area is super brittle which makes it very annoying to work with, I wish we could add in some automated tests which can catch more of this stuff. Alas, we should at least take all the regressions and add them as test steps for the next try. |
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.3-11 🚀
|
Details
Fixed Issues
$ #30042
PROPOSAL: $ #30042 (comment)
Tests
Create an iouReport
request action preview (requestor view)
request action preview (payer view)
DMs:
owed iouReport being previewed (Agata view - DM)
owed iouReport being previewed (Tom view - DM)
Create an expenseReport
Request action preview (admin viewer)
Request action preview (requestor viewer)
owed expenseReport being previewed (member view - workspace chat)
owed expenseReport being previewed (admin view - workspace chat)
Pay an iouReport
paid action preview (requestor view)
paid action preview (payer view)
paid iouReport being previewed (Agata view - DM)
paid iouReport being previewed (Tom view - DM)
Pay an expenseReport
Paid action preview (admin viewer)
paid action preview (requestor viewer)
paid expenseReport being previewed (member view - workspace chat)
paid expenseReport being previewed (admin view - workspace chat)
Add a comment
comment preview (sender view)
comment preview (receiver view)
comment preview (comment sender viewer)
comment preview (comment receiver viewer)
Offline tests
N/A
QA Steps
Same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
EXPENSE REPORT
Request action preview (admin viewer)
Request action preview (requestor viewer)
Paid action preview (admin viewer)
paid action preview (requestor viewer)
comment preview (comment sender viewer)
comment preview (comment receiver viewer)
IOU REPORT
request action preview
paid action preview
comment preview
DM / WORKSPACE CHAT
owed iouReport being previewed (Agata view - DM)
owed iouReport being previewed (Tom view - DM)
paid iouReport being previewed (Agata view - DM)
paid iouReport being previewed (Tom view - DM)
owed expenseReport being previewed (member view - workspace chat)
owed expenseReport being previewed (admin view - workspace chat)
paid expenseReport being previewed (member view - workspace chat)
paid expenseReport being previewed (admin view - workspace chat)
MacOS: Desktop
all examples like in MacOS: Chrome / Safari