[Awaiting C+ for payment][$500] Profile - Private notes doesn't appear on profile when navigate via Share code link

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.35-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: applause-Internal Team
Slack conversation:

Action Performed:

Precondition:
User A have a existing conversation with User B

1. While logged out User A navigate to conversation with User B via Share code link
2. Take a note of the Profile modal

Expected Result:

Since there is a existing conversation between users - on the Profile modal Private notes should be present

Actual Result:

Private notes does not appear on existing conversation when navigate logged out via Share code link

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6364618_1706866486295.az_recorder_20240201_222649.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0145bd5b64b244e0b6
• Upwork Job ID: 1753374296548478976
• Last Price Increase: 2024-02-23
• Automatic offers:
• shubham1206agra | Reviewer | 0
• dukenv0307 | Contributor | 0

Issue Owner

Current Issue Owner: @shubham1206agra

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0145bd5b64b244e0b6

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @shubham1206agra (External)

[melvin-bot]

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[izarutskaya]

We think that this bug might be related to #vip-vsb
CC @quinthar

[FitseTLT]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Private notes doesn't appear on profile when navigate via Share code link

What is the root cause of that problem?

The problem lies in how we are getting the reportID for the 1to1 chat report here as it depends on the reports list in onyx

App/src/pages/ProfilePage.js

Lines 281 to 287 in 9d4c634

	key: ({route, session}) => {
	const accountID = Number(lodashGet(route.params, 'accountID', 0));
	const reportID = lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '');
	if ((session && Number(session.accountID) === accountID) || Session.isAnonymousUser() || !reportID) {
	return null;
	}
	return `${ONYXKEYS.COLLECTION.REPORT}${reportID}`;

As getChatByParticipants depends on the reports from onyx if it is not yet fetched from BE when the profile page is rendered it will result in null reportID and privates notes not displayed

App/src/pages/ProfilePage.js

Lines 238 to 239 in 9d4c634

	{!_.isEmpty(props.report) && (
	<MenuItem

Similarly notification preference is not shown too

App/src/pages/ProfilePage.js

Lines 135 to 136 in e791ecc

	const shouldShowNotificationPreference = !_.isEmpty(props.report) && props.report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
	const notificationPreference = shouldShowNotificationPreference ? props.translate(`notificationPreferencesPage.notificationPreferences.${props.report.notificationPreference}`) : '';

What changes do you think we should make in order to solve the problem?

In cases where the report is empty we need to subscribe to the reports collection (onyxSubscribe) and when it is successfully populated in onyx we need to recalculate via getChatByParticipants to get report when the reports list is successfully fetched from BE in Profile Page.

What alternative solutions did you explore? (Optional)

We can also think of having the reportID data in the route in that case we don't depend on the getChatByParticipants to get the reportID

[dukenv0307]

Proposal

Please re-state the problem that we are trying to solve in this issue.

• Profile - Private notes doesn't appear on profile when navigate via Share code link

What is the root cause of that problem?

• In case of report data, previousKey and newKey in here are always the same, that leads to the logic here never runs. So the report data in ProfilePage is always empty. Below is detail:
• The report key is a function, with the input is {route, session} and return the reportID. But this function also needs a reports.
• We get the previousKey and newKey based on the prop and state of the component, but in this case, the reports does not come from prop and state, instead it is a global variable, so Str.result(mapping.key, {...prevProps, ...prevOnyxDataFromState}) and Str.result(mapping.key, {...this.props, ...onyxDataFromState}) are always the same.

What changes do you think we should make in order to solve the problem?

• Based on the above RCA, we can fix the issue by updating the report key to use the reports from component `s props and state rather than global variable.
• For example, we can update the getChatByParticipants to:

function getChatByParticipants(newParticipantList: number[], reports = allReports): OnyxEntry<Report> {

so we can pass the reports data to it.

• Then, update the below logic:
  

  App/src/pages/ProfilePage.js

  Lines 284 to 304 in 0ae07be

  	export default compose(
  	withLocalize,
  	withOnyx({
  	personalDetails: {
  	key: ONYXKEYS.PERSONAL_DETAILS_LIST,
  	},
  	session: {
  	key: ONYXKEYS.SESSION,
  	},
  	report: {
  	key: ({route, session}) => {
  	const accountID = Number(lodashGet(route.params, 'accountID', 0));
  	const reportID = lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '');
  	if ((session && Number(session.accountID) === accountID) || Session.isAnonymousUser() || !reportID) {
  	return null;
  	}
  	return `${ONYXKEYS.COLLECTION.REPORT}${reportID}`;
  	},
  	},
  	}),
  	)(ProfilePage);

  to:

export default compose(
    withLocalize,
    withOnyx(
        {
            reports: {
                key: ONYXKEYS.COLLECTION.REPORT,
            },
            personalDetails: {
                key: ONYXKEYS.PERSONAL_DETAILS_LIST,
            },
            session: {
                key: ONYXKEYS.SESSION,
            },
            report: {
+               key: ({route, session, reports}) => {
                    const accountID = Number(lodashGet(route.params, 'accountID', 0));
+                  const reportID = lodashGet(ReportUtils.getChatByParticipants([accountID], reports), 'reportID', '');
                    if ((session && Number(session.accountID) === accountID) || Session.isAnonymousUser() || !reportID) {
                        return null;
                    }
                    return `${ONYXKEYS.COLLECTION.REPORT}${reportID}`;
                },
            },
        },
    ),
)(ProfilePage);

• Also, we can use the selector in the above, which will just get all the properties that will be used in getChatByParticipants

What alternative solutions did you explore? (Optional)

• Also, we can update:
  https://github.com/Expensify/react-native-onyx/blob/13936aee27bb0cb8c98cb6bf3941044f3d5958ce/lib/withOnyx.js#L133-L137
  to:

+                  if (previousKey !== newKey || !this.activeConnectionIDs[previousKey]) {
                        Onyx_1.default.disconnect(this.activeConnectionIDs[previousKey], previousKey);
                        delete this.activeConnectionIDs[previousKey];
                        this.connectMappingToOnyx(mapping, propName, newKey);
                    }

[melvin-bot]

@miljakljajic, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

[miljakljajic]

reviewing proposals still

[shubham1206agra]

@miljakljajic I am waiting for a better proposal. The current proposals add listeners to all reports, which I think is not a good approach.

[FitseTLT]

@shubham1206agra The data the page gets from the route is accountID and it needs to get the DM report linked to that participant and if reports data is not ready we will definitely need to recalculate the getChatByParticipants when the data reports is ready. So what I am proposing is just a useEffect that will be run once on render and will only subscribe to the report collection if report is not ready at that time (what is needed in the case of this current issue) and early return for the case the report is ready and once the reports collection is ready we will populate the report and will disconnect inside the callback. I don't see an inefficiency in it.

[shubham1206agra]

Can you give me a test branch for this?

[FitseTLT]

Can you give me a test branch for this?

Will do shortly

[dukenv0307]

@shubham1206agra Do you have any feedback about my proposal?

[shubham1206agra]

I think I have same feedback as #35654 (comment). I just don't need to necessarily listen to all reports after initial rendering.

[FitseTLT]

Here it is @shubham1206agra

[dukenv0307]

@shubham1206agra I updated my alternative solution based on the RCA

[FitseTLT]

@shubham1206agra be notified that made changes on the test branch 👍

[shubham1206agra]

@FitseTLT I still don't like this solution.

@dukenv0307 We cannot update the Onyx logic. Maybe design a selector for all reports (Not sure if it prevents re-rendering on any change in the collection).

[dukenv0307]

@shubham1206agra

design a selector for all reports

Can you give me more details about this one? The current behavior is that the report key here has correct key but the Onyx does not listen to that case

[shubham1206agra]

Maybe see the SidebarLinkData component.

[dukenv0307]

@shubham1206agra yep. We just need to get all the properties that will be used in getChatByParticipants

[trjExpensify]

Sure! Awaiting the regression period to run down melv, chill.

[jasperhuangg]

^ Same

[trjExpensify]

👋 Checklist time please!

[shubham1206agra]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR: Add notification preference for all the reports and update the design for the group chats #28200
• [@shubham1206agra] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/28200/files#r1516053165
• [@shubham1206agra] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not Required
• [@shubham1206agra] Determine if we should create a regression test for this bug. No
• [@shubham1206agra] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@miljakljajic] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[shubham1206agra]

@trjExpensify Can you process payment here, please?

[trjExpensify]

Yep! Payment summary as follows:

• $500 to @dukenv0307 for the fix. - paid
• $500 to @shubham1206agra for C+ review. - needs payment, see below. - Now paid

@dukenv0307 I've paid you. @shubham1206agra doesn't look like you've accepted the offer yet.

[shubham1206agra]

@trjExpensify Can you hold my payment temporarily as per https://expensify.slack.com/archives/C02NK2DQWUX/p1710150138788529?

[trjExpensify]

Yep, please comment here when your ready. Dropping to Weekly and updating the title.

[shubham1206agra]

@trjExpensify I have discussed this internally. You may close this issue as I am keeping track of payment internally and will ask to pay once the issue is resolved. Just write in the payment summary that I have not been paid yet.

[trjExpensify]

Sounds good, closing!

[shubham1206agra]

@trjExpensify We can process payment here now.

[trjExpensify]

Sent a new offer.

[shubham1206agra]

Accepted

[trjExpensify]

Paid!