-
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
fix reply in thread does not have from link in header #24113
Conversation
@fedirjh Could you please review PR? |
@StevenKKC We have this warning in the Android native |
@fedirjh I can't reproduce it. Could you please record video? |
@fedirjh We should fix this warning, right? |
@fedirjh I have committed. Please check again. |
Reviewer Checklist
Screenshots/VideosMobile Web - SafariCleanShot.2023-08-04.at.12.21.44.mp4 |
@StevenKKC For QA let's add another test for Policy expense reports. |
@fedirjh Seems like I have no permission for the policy expense reports. After create a workspace, if open the report, BE returns as below. |
@StevenKKC I think you have to ask for policy expense beta access in Slack #expensify-open-source channel. |
@fedirjh Updated test steps. |
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.
Looks good to me and tests well.
🎀 👀 🎀 C+ reviewed
@bondydaa Could you please review PR? |
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.
I'm going to ask about my comment in slack since I'm not the most up to date on new dot things to make sure I haven't missed or confused something but otherwise things look good. thank you for testing the other spots like the anonymous footer as well!
@@ -18,7 +18,7 @@ const propTypes = { | |||
}).isRequired, | |||
|
|||
/** parent Report ID */ | |||
parentReportID: PropTypes.string, | |||
parentReportID: PropTypes.oneOfType([PropTypes.string, PropTypes.number]), |
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.
Hmm I thought reportID were always supposed to be strings when they got returned from the API... if this isn't the case something might be broken in the API.
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.
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.
Please check #24113 (comment)
BE returns number type of parentReportID for the task report.
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.
I think we are just missing cases in the backend where these properties should be converted to strings... like parentReportID
For example, the reportID
in a report is a string:
We should be consistent on that.
Now... since the backend is wrong, I'm not sure what do in your case... should we fix the backend and put this on hold until that is done? or we fix this in a follow up when we update the backend?
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.
The changes in this commit are not required for this PR, it was added to address this warning, Since this PR does not cause the warning, I think we should revert the changes and make a fix in the backend.
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.
Here is a link to the related bug: https://expensify.slack.com/archives/C049HHMV9SM/p1691035111039979.
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.
@fedirjh Let me revert this commit, what do you think?
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.
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.
Yeah let's revert that, we have a PR open right now to fix this so that the BE will return strings.
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.
@bondydaa I have reverted last commit. Can you please review again?
cc @bondydaa little bump |
@bondydaa Friendly bump |
doesn't look like the back end PR is merged yet https://github.com/Expensify/Web-Expensify/pull/38444 I think this is okay to merge though, it will just throw a warning if the type is incorrect right, not crash the app or anything? |
@bondydaa It will just throw a warning. Could you please merge this branch? |
✋ 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/bondydaa in version: 1.3.56-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.56-24 🚀
|
Details
Fixed Issues
$ #23492
PROPOSAL: #23492 (comment)
Tests
Offline tests
N/A
QA Steps
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
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)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
Web
Web.webm
Mobile Web - Chrome
Mobile Web - Safari
Desktop
Desktop.webm
iOS
Android