-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Track expense/self dm #37165
Track expense/self dm #37165
Conversation
Thanks for this PR - I did some testing with my account that has a self-DM set up. There's a couple thing I noticed Screen.Recording.2024-02-26.at.10.08.44.AM.mov
|
@ishpaul777 Please fix conflicts and address #37165 (comment). |
fixed in 5d7a997
still have to investigate can't reproduce it with testing workaround but i am lookin into the addComment logic for better understanding
fixed in c8ae559 |
@thienlnam do you mind taking a look again 🙇♂️ |
Typescript checks failing is unrelated |
src/libs/OptionsListUtils.ts
Outdated
@@ -1460,6 +1464,7 @@ function getOptions( | |||
const isTaskReport = ReportUtils.isTaskReport(report); | |||
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report); | |||
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report); | |||
const isSelfDM = ReportUtils.isSelfDM(report); | |||
const accountIDs = report.visibleChatMemberAccountIDs ?? []; |
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.
comment for reference
Co-authored-by: Pavlo Tsimura <[email protected]>
… into trackExpense/selfDM
Co-authored-by: Pavlo Tsimura <[email protected]>
Co-authored-by: Pavlo Tsimura <[email protected]>
Co-authored-by: Pavlo Tsimura <[email protected]>
… into trackExpense/selfDM
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 and tests well.
@ishpaul777 please fix lint and we'll be good to go 👍 |
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!!
✋ 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/thienlnam in version: 1.4.47-0 🚀
|
@@ -166,7 +169,7 @@ function HeaderView(props) { | |||
), | |||
); | |||
|
|||
const canJoinOrLeave = isChatThread || isUserCreatedPolicyRoom || canLeaveRoom; | |||
const canJoinOrLeave = isChatThread || !isSelfDM || isUserCreatedPolicyRoom || canLeaveRoom; |
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 like this condition caused regression #37709
@ishpaul777 could you please take a look
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.47-10 🚀
|
We missed a case to update the LHN message for self DM |
Details
Tasks in Scope:
Fixed Issues
$ #36361
PROPOSAL:
Tests
Adding the reportName for this chat 'Your Name (you)'
+ (you)
+ (you)
+ (you)
Removing the option to 'Request/Send Money' from the chat
Preventing this self-dm from showing up in the participant selectors (except tasks)
Determining how the details page of the self DM will look.
-Verify it should looks same a 1:1 chat details page except all of the option like message/private notes/notification preference/settings/members should be hidden and their deeplinks should show not found page
Offline tests
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)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
Screen.Recording.2024-03-02.at.6.59.28.AM.mov
Android: mWeb Chrome
Screen.Recording.2024-03-02.at.6.46.12.AM.mov
iOS: Native
Screen.Recording.2024-03-02.at.6.30.57.AM.mov
iOS: mWeb Safari
Screen.Recording.2024-03-02.at.6.36.11.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-03-02.at.6.12.36.AM-1.mov
MacOS: Desktop
Screen.Recording.2024-03-02.at.7.03.06.AM-1.mov