[HOLD for payment 2023-11-17] [$250] Clean the usage of double onyx call on profile page

[techievivek]

Problem

We currently have used a double onyx call on the profile page to subscribe to the DM chat report in order for notification preference to update correctly for the profile page. This was reported during PR review here: #28200 (comment)

App/src/pages/ProfilePage.js

Lines 306 to 317 in a63f853

	// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
	withOnyx({
	report: {
	key: ({route, session}) => {
	const accountID = Number(lodashGet(route.params, 'accountID', 0));
	if (Number(session.accountID) === accountID || Session.isAnonymousUser()) {
	return null;
	}
	return `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '')}`;
	},
	},
	}),

Discussed here: #28200 (comment)

Why is this important?

Using a double Onyx subscription for any component is not a recommended practice so it would be nice to clean that up. Moreover, we have now enabled the notification preference option for all the chat reports so maybe we can create a separate component for the notification preference page and reuse that across various pages.

Solution

Create a separate component for notification preference and get rid of the double onyx call on the profile page. Feel free to come up with your own solutions.

Here is a potential solution to the problem: #28200 (comment)

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013768a6222352d168
• Upwork Job ID: 1711676109112000512
• Last Price Increase: 2023-10-24
• Automatic offers:
• pradeepmdk | Contributor | 27435339

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013768a6222352d168

[melvin-bot]

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

[melvin-bot]

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

[techievivek]

Bug0 triage checklist is not required for this GH, as this is just a code improvement issue.

[ghost]

Dibs

[alexxxwork]

Is it available without a proposal? I would like to take it.

[techievivek]

I would appreciate it if you can share the proposal beforehand.

[pradeepmdk]

Proposal

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

Clean the usage of double onyx call on profile page

What is the root cause of that problem?

double onyx is used so we should clean that make only one and create a new component for Notificaion preference

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

1. App/src/pages/ProfilePage.js

   Lines 306 to 317 in a63f853

   	// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
   	withOnyx({
   	report: {
   	key: ({route, session}) => {
   	const accountID = Number(lodashGet(route.params, 'accountID', 0));
   	if (Number(session.accountID) === accountID || Session.isAnonymousUser()) {
   	return null;
   	}
   	return `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '')}`;
   	},
   	},
   	}),

   
   we should remove the second one and move to the above onyx after the session key
   recently react-native-onyx allowed to this https://github.com/Expensify/react-native-onyx/pull/385/files

before proceeding with this we should upgrade latest version of react-native-onyx

2. we need a new component for notification preference to use across page
   in component folder we can create notification preference and move all the logic from profile page. and on that component we can subscribe onyx as like profile page

[techievivek]

@burczu Can you please have a look at the above proposals. Thanks.

[techievivek]

Gentle bump @burczu

[burczu]

@techievivek I'm sorry, I was busy reviewing PR's - I'll get back to checking proposals soon.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[techievivek]

Friendly bump @burczu

[techievivek]

Not overdue, assigned the GH to @pradeepmdk

[burczu]

Just to inform, I'll be ooo for the next few days and we'll be back on Monday, November 6th.

[melvin-bot]

@johncschuster @burczu @pradeepmdk @techievivek this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.97-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• remove double onyx in profile page  #30694

If no regressions arise, payment will be issued on 2023-11-17. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @burczu does not require payment (Contractor)
• @pradeepmdk requires payment offer (Contributor)

[melvin-bot]

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:

• [@burczu] The PR that introduced the bug has been identified. Link to the PR:
• [@burczu] 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:
• [@burczu] 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:
• [@burczu] Determine if we should create a regression test for this bug.
• [@burczu] 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.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[pradeepmdk]

bump for payment

[pradeepmdk]

@johncschuster bump for payment

[techievivek]

Gentle bump @johncschuster ^

[burczu]

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:

• [@burczu] The PR that introduced the bug has been identified. Link to the PR:

This was just refactor of an existing feature.

• [@burczu] 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:

n/a

• [@burczu] 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:

n/a

• [@burczu] Determine if we should create a regression test for this bug.

As this was just a refactor, I guess we should expect regression tests for this feature: #27394 should cover testing this issue.

• [@burczu] 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.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[pradeepmdk]

@johncschuster bump for payment

[pradeepmdk]

@techievivek can u help me for payment here

[johncschuster]

Sorry I dropped the ball, @pradeepmdk. I'll get payment issued this morning.

[pradeepmdk]

Thank you @johncschuster

[johncschuster]

Payment has been issued! Thanks for your patience!