-
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 recents in participants page based on action type #35431
show recents in participants page based on action type #35431
Conversation
thanks! thats your eta to get this ready for a review? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…ature-show-recents-based-on-action-type
The PR is ready for review. I have documented the implementation details in Post review, I will complete testing for the remaining environments too. Thanks for your patience. |
@eVoloshchak 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] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
src/libs/OptionsListUtils.ts
Outdated
@@ -99,6 +99,7 @@ type GetOptionsConfig = { | |||
includePolicyTaxRates?: boolean; | |||
policyTaxRates?: PolicyTaxRateWithDefault; | |||
transactionViolations?: OnyxCollection<TransactionViolation[]>; | |||
actionTypeForParticipants?: 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.
type ActionType = ValueOf<typeof CONST.REPORT.TYPE | typeof CONST.IOU.TYPE | typeof CONST.IOU.REQUEST_TYPE>;
actionTypeForParticipants?: string; | |
actionTypeForParticipants?: ActionType; |
src/libs/TransactionUtils.ts
Outdated
Object.values(allTransactions ?? {}) | ||
.filter( | ||
(transaction): transaction is Transaction => | ||
transaction != null && (actionType === CONST.IOU.TYPE.SPLIT ? isSplitRequest(transaction) : getRequestType(transaction) === actionType), |
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.
This is a little bit hard to read, I think a helper method (something like getTransactionActionType
, essentially a combination of isSplitRequest
and getRequestType
) would be useful here
Bug:
Screen.Recording.2024-02-01.at.20.45.46.mov |
@rojiphil yeah that is what we are looking for, however I agree that to properly capture the impact we might need more complex scenario here. Can you please resolve the conflicts and retest? |
…ature-show-recents-based-on-action-type
@rojiphil more conflicts |
This comment was marked as outdated.
This comment was marked as outdated.
Resolved the conflicts and retested too. It needed minor fixes but the core logic remains the same. The reassure performance tests also passed well. LGTM. |
@rojiphil, Test 1: Task Creation is failing for me Screen.Recording.2024-04-08.at.19.22.16.mov |
@eVoloshchak That’s an interesting observation. I was not able to get this problem as I was not that quick in coming back to the task creation to check if the assignee participants page is updated. For some weird reason, here it looks like the reports do not immediately include the newly created task report which is why the assignee participants page is not updated. However, this is only observed for a short period of time. It may be that the API response/pusher updates will later populate the reports with the newly created report and the assignee participants page will show the updated participants. Although I think we should fix this I also think this falls outside the scope of our current work here. Also, as it is momentary in nature it may also qualify as a super-edge case. Anyway, I am making a note of this in the PR description. Does this make sense? 34227-delayed-update.mp4 |
Can confirm it does work after some time (or a page refresh). |
@rojiphil, gentle bump on this question |
I am not quite sure how |
It looks like the option list is not getting updated for the intermediate reports and gets updated only for the last updated report when there are multiple reports updated at the same time. In our use case here, we update two reports at the same time i.e. @eVoloshchak Can you please confirm if you share a similar understanding here? |
…ature-show-recents-based-on-action-type
@rojiphil, thank you for the thorough explanation!
This is the actual bug we need to fix. Was it introduced by #38207? |
@eVoloshchak Yes. That's correct. The performance improvement via caching search options caused this. The |
@eVoloshchak @mountiny
Details
Currently, the
recent
list on the participants’ page displays the participant details of the most recently changed reports at the top. This PR, now, shows the participant details of the most recently changed reports at the top only when they were involved in theaction
for which these reports are shown. For example, when user A generates ascan
receipt money request in a 1:1 DM for User B, the recent list shows participant B in the recent list since the 1:1 DM chat report has a recent history of ascan
money receipt.How do we do this?
For Money Request action types:
We have three types of money requests i.e.
manual
,scan
, anddistance
which are the three types of action for money requests. These actions are initiated by the user via thetab
selection. Further, formanual
andscan
money requests, the user can additionally trigger a fourth action i.e.split
action by clicking on thesplit
button on the participants’ page.For identifying the reports based on these four types the action, we rely on the
transactions
available locally in Onyx as follows:Split: When the
comment source
is set tosplit
within atransaction
.Scan: When there is a valid
receipt
within a transactionDistance: When the transaction contains a
customUnit
type and custom unit name asDistance
Manual: None of the above
For transactions of a given action type, we identify their respective
chat reports
i.e. 1:1 DM Chat Repot or Workspace Chat Report via the money request report. The most recent of these chat reports will be displayed.For Task action type:
During task creation, we display the participants’ page during the assignment of the task. This is what makes the
task
action type.Here, we identify the parent
chat report
of all tasks available within Onyx. The most recent of these chat reports are displayed in the recent list.Note
Since we depend on
transactions
andtask reports
to find the most recent chat reports, it is possible that we do not find the most recent chat report just because the dependanttransactions
andtask reports
are not available locally in Onyx even though the chat report is locally available. This scenario occurs when the remote user generates a transaction/task that is not yet accessed locally by the local user or during a fresh sign-in. Further, it may be possible that thetransaction/task report
and the parentchat report
are locally present but the intermediate reports are not present locally. Suchchat reports
will be missing from the recent list as they could not be traced back from thetransaction/task report
.Conclusion: Currently, we will use when its available as there is some BE planning that is pending as mentioned here
After creation of a new task/money request, if we quickly come back to check if the participants page has been updated, we may not see the change immediately. However, this is only observed for a short period. It looks like the API response/pusher updates will later populate the reports with the newly created report and the assignee participants page will show the updated participants. So, if this behavior is observed, please give few moments and come back again to verify. The comment here also shares a video that demonstrates this behavior.
Fixed Issues
$ #34227
PROPOSAL: #34227 (comment)
Tests
Precondition: For Money Requests, ensure that all previous money requests are paid.
Test 1: Task Creation
Next
Assignee
to open participants pageTest 2: Manual Money Request
Manual
money request for User B in 1:1 Chat ReportTest 3: Split Money Request
Split
money request with User B in 1:1 Chat ReportTest 4: Scan Receipt Money Request
Scan
receipt money request for User D in 1:1 Chat ReportTest 5: Distance Money Request
Distance
money request in Workspace XTest 6: Add Comment to a Chat Report that has a previous manual money request
Test 7: Test for default recent list
Precondition: Have split requests with few participants in User A’s account
Note: All the above 7 tests can be performed by initiating a money request/task creation from remote user. However, the only thing to take care of is that the local user should open the respective transaction/task.
Offline tests
Same as the Steps in Tests Section.
QA Steps
Same as the Steps in Tests Section.
Notes
notes
section in PR description for exceptions.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
Web - Safari
Test 1: Task Creation
34227-web-safari-task.mp4
Test 2: Manual Money Request
34227-web-safari-manual.mp4
Test 3: Split Money Request
34227-web-safari-split.mp4
Test 4: Scan Receipt Money Request
34227-web-safari-scan.mp4
Test 5: Distance Money Request
34227-web-safari-distance.mp4
Test 6: Add Comment to a Chat Report that has a previous manual money request
34227-web-safari-comment.mp4
Test 7: Test for default recent list
34227-web-safari-default.mp4
Mobile Web - Safari
Test 1: Task Creation
34227-mweb-safari-T01.mp4
Test 2: Manual Money Request
34227-mweb-safari-T02.mp4
Test 3: Split Money Request
34227-mweb-safari-T03.mp4
Test 4: Scan Receipt Money Request
34227-mweb-safari-T04.mp4
Test 5: Distance Money Request
34227-mweb-safari-T05.mp4
Test 6: Add Comment to a Chat Report that has a previous manual money request
34227-mweb-safari-T06.mp4
Test 7: Test for default recent list
34227-mweb-safari-T07.mp4
Desktop
Test 1: Task Creation
34227-desktop-T01.mp4
Test 2: Manual Money Request
34227-desktop-T02.mp4
Test 3: Split Money Request
34227-desktop-T03.mp4
Test 4: Scan Receipt Money Request
34227-desktop-T04.mp4
Test 5: Distance Money Request
34227-desktop-T05.mp4
Test 6: Add Comment to a Chat Report that has a previous manual money request
34227-desktop-T06.mp4
Test 7: Test for default recent list
34227-desktop-T07.mp4
iOS
Test 1: Task Creation
34277-ios-native-T01.mp4
Test 2: Manual Money Request
34227-ios-native-T02.mp4
Test 3: Split Money Request
34277-ios-native-T03.mp4
Test 4: Scan Receipt Money Request
34227-ios-native-T04.mp4
Test 5: Distance Money Request
34227-ios-native-T05.mp4
Test 6: Add Comment to a Chat Report that has a previous manual money request
34227-desktop-T06.mp4
Test 7: Test for default recent list
34227-ios-native-T07.mp4
Android
Test 1: Task Creation
34227-android-native-T01.mp4
Test 2: Manual Money Request
34227-android-native-T02.mp4
Test 3: Split Money Request
34227-android-native-T03.mp4
Test 4: Scan Receipt Money Request
34227-android-native-T04.mp4
Test 5: Distance Money Request
34227-android-native-T05.mp4
Test 6: Add Comment to a Chat Report that has a previous manual money request
34227-android-native-T06.mp4
Test 7: Test for default recent list
34227-android-native-T07.mp4
Mobile Web - Chrome
Test 1: Task Creation
34227-mweb-chrome-T01.mp4
Test 2: Manual Money Request
34227-mweb-chrome-T02.mp4
Test 3: Split Money Request
34227-mweb-chrome-T03.mp4
Test 4: Scan Receipt Money Request
34227-mweb-chrome-T04.mp4
Test 5: Distance Money Request
34227-mweb-chrome-T05.mp4
Test 6: Add Comment to a Chat Report that has a previous manual money request
34227-mweb-chrome-T06.mp4
Test 7: Test for default recent list
34227-mweb-chrome-T07.mp4