Show a growl and redirect when navigating to a chat we can't access #4018

rdjuric · 2021-07-13T22:06:20Z

Details

Modifies our fetchChatReportsByIDs action to receive a boolean flag shouldRedirectIfInacessible. With this flag set to true, the action will redirect to Concierge and display a Growl if it was asked to fetch a single report and that report was inacessible.

We call a report inacessible if our API answers with a jsonCode of 404 when we try to fetch it.

In our ReportActionsView, check if the current report has a reportID saved locally. If it doesn't, try to fetch the report with the shouldRedirectIfInacessible flag set to true.

Fixed Issues

$ #3810

Tests

QA Steps

Try to access a report you don't have access to - like /r/75205749
A growl indicating an error should shows up
You should be redirected to the Concierge chat

Tested On

Screenshots

Web

web.mov

Mobile Web

mWeb.mp4

Desktop

iOS

Android

rdjuric · 2021-07-14T18:23:12Z

Modified the original proposal to match your comment more closely @marcaaron.

I'm using navigateToConciergeChat instead of navigating to ROUTES.HOME as the inaccessible report would still show up after navigating to to /.

I think that happens because the report was still saved under CURRENTLY_VIEWED_REPORTID. Navigating to Concierge looked better and more foolproof than modifying our logic for saving the last viewed report.

rdjuric · 2021-07-14T23:05:43Z

Updated to use the new GetReportSummaryList

marcaaron · 2021-07-14T23:42:17Z

@rdjuric Ok that sounds like a good solution to me, thanks!

marcaaron

Just a few comments. Nice work so far 🎉

marcaaron · 2021-07-14T23:44:58Z

src/libs/actions/Report.js

+
+            // If we receive a 404 response while fetching a single report, treat that report as inacessible.
+            if (jsonCode === 404 && chatList.length === 1) {
+                throw new Error('inacessible');


Can we perhaps create a constant for this and maybe give it a slightly more descriptive error something like this?

const CONST = { ..., ERROR: { INACCESSIBLE_REPORT: 'Report not found', }, };

Then we can use CONST.ERROR.INACCESSIBLE_REPORT in these two locations.

marcaaron · 2021-07-14T23:46:24Z

src/libs/actions/Report.js

+        })
+        .catch((err) => {
+            if (err.message === 'inacessible' && shouldRedirectIfInacessible) {
+                Log.info('[Report] Report data is inacessible.', true);


Let's maybe get rid of this log since it will actually print to the server and we don't need it to do that in this case.

rdjuric · 2021-07-15T00:35:26Z

Thanks for the feedback! Updated @marcaaron.

rdjuric · 2021-07-19T18:17:43Z

Can we merge this one @marcaaron?

marcaaron · 2021-07-19T18:22:10Z

src/libs/actions/Report.js

            Log.info('[Report] successfully fetched report data', true);
            fetchedReports = reportSummaryList;
+
+            // If we receive a 404 response while fetching a single report, treat that report as inacessible.
+            if (jsonCode === 404 && chatList.length === 1) {


not sure checking the chatList is necessary on second thought

we should be checking shouldRedirectIfInacessible when we throw this error

Makes sense. If we're controlling the redirect via the shouldRedirectIfInacessible param, checking the chatList is a weird restriction.

marcaaron · 2021-07-19T18:23:45Z

src/libs/actions/Report.js

- *
+ * Fetches chat reports when provided a list of chat report IDs.
+ * If the shouldRedirectIfInacessible flag is set, we redirect to the Concierge chat
+ * when fetching a single chat that is inacessible.


Suggested change

* when fetching a single chat that is inacessible.

* when we find an inaccessible chat.

marcaaron · 2021-07-19T18:24:30Z

src/libs/actions/Report.js

@@ -368,6 +375,13 @@ function fetchChatReportsByIDs(chatList) {
            PersonalDetails.getFromReportParticipants(Object.values(simplifiedReports));

            return _.map(fetchedReports, report => report.reportID);
+        })
+        .catch((err) => {
+            if (err.message === CONST.REPORT.ERROR.INACCESSIBLE_REPORT && shouldRedirectIfInacessible) {


Suggested change

if (err.message === CONST.REPORT.ERROR.INACCESSIBLE_REPORT && shouldRedirectIfInacessible) {

if (err.message === CONST.REPORT.ERROR.INACCESSIBLE_REPORT) {

marcaaron · 2021-07-19T18:24:47Z

src/libs/actions/Report.js

            Log.info('[Report] successfully fetched report data', true);
            fetchedReports = reportSummaryList;
+
+            // If we receive a 404 response while fetching a single report, treat that report as inacessible.
+            if (jsonCode === 404 && chatList.length === 1) {


Suggested change

if (jsonCode === 404 && chatList.length === 1) {

if (jsonCode === 404 && shouldRedirectIfInacessible) {

marcaaron · 2021-07-19T18:27:27Z

src/pages/home/report/ReportActionsView.js

@@ -107,6 +108,9 @@ class ReportActionsView extends React.Component {

    componentDidMount() {
        AppState.addEventListener('change', this.onVisibilityChange);
+        if (!this.props.report.reportID) {


It feels like there's a special significance to this not existing. Can we add a comment above to maybe explain why this does not exist and why it's being called with this parameter? Maybe something like

If the reportID is not found then we have either not loaded this chat or the user is unable to access it. We will attempt to fetch it and redirect if still not accessible.

rdjuric · 2021-07-19T21:13:48Z

Updates made @marcaaron

marcaaron

LGTM

marcaaron · 2021-07-19T21:21:58Z

Nice ! Thanks @rdjuric

OSBotify · 2021-07-19T21:22:39Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-07-23T21:35:43Z

🚀 Deployed to staging in version: 1.0.79-5🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-07-28T21:09:11Z

🚀 Deployed to production in version: 1.0.80-2🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

rdjuric added 7 commits July 13, 2021 19:03

adds callbackIfInacessible to fetchChatReportsById

49b293c

fixes comment

fd089ab

changes callback to direct redirect in the fetch

650aa1f

navigate to concierge, translated growl

2a365ab

lint fixes and small improvements

2611403

fetch only if local chat has no reportID

80530b9

lint

7fb9038

rdjuric marked this pull request as ready for review July 14, 2021 18:18

rdjuric requested a review from a team as a code owner July 14, 2021 18:18

MelvinBot requested review from marcaaron and removed request for a team July 14, 2021 18:18

rdjuric added 2 commits July 14, 2021 19:58

resolution for conflict

e5d2979

lint fixes

85749e3

marcaaron assigned rdjuric Jul 14, 2021

marcaaron requested changes Jul 14, 2021

View reviewed changes

minor changes

e7f9d50

rdjuric requested a review from marcaaron July 15, 2021 00:35

marcaaron requested changes Jul 19, 2021

View reviewed changes

rdjuric added 2 commits July 19, 2021 17:32

updates to Report and comment to ReportActionsView

0ed5ef4

lint fixes

563393e

rdjuric requested a review from marcaaron July 19, 2021 21:13

marcaaron approved these changes Jul 19, 2021

View reviewed changes

marcaaron merged commit 4749488 into Expensify:main Jul 19, 2021

This was referenced Jul 20, 2021

Deploy Checklist: Expensify.cash 2021-07-14 #4046

Closed

Deploy Checklist: Expensify.cash 2021-07-23 #4198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a growl and redirect when navigating to a chat we can't access #4018

Show a growl and redirect when navigating to a chat we can't access #4018

rdjuric commented Jul 13, 2021 •

edited

Loading

rdjuric commented Jul 14, 2021

rdjuric commented Jul 14, 2021

marcaaron commented Jul 14, 2021

marcaaron left a comment

marcaaron Jul 14, 2021 •

edited

Loading

marcaaron Jul 14, 2021

marcaaron Jul 14, 2021

rdjuric commented Jul 15, 2021

rdjuric commented Jul 19, 2021

marcaaron Jul 19, 2021

rdjuric Jul 19, 2021

marcaaron Jul 19, 2021

marcaaron Jul 19, 2021

marcaaron Jul 19, 2021

marcaaron Jul 19, 2021

rdjuric commented Jul 19, 2021

marcaaron left a comment

marcaaron commented Jul 19, 2021

OSBotify commented Jul 19, 2021

OSBotify commented Jul 23, 2021

OSBotify commented Jul 28, 2021

	* when fetching a single chat that is inacessible.
	* when we find an inaccessible chat.

	if (err.message === CONST.REPORT.ERROR.INACCESSIBLE_REPORT && shouldRedirectIfInacessible) {
	if (err.message === CONST.REPORT.ERROR.INACCESSIBLE_REPORT) {

	if (jsonCode === 404 && chatList.length === 1) {
	if (jsonCode === 404 && shouldRedirectIfInacessible) {

Show a growl and redirect when navigating to a chat we can't access #4018

Show a growl and redirect when navigating to a chat we can't access #4018

Conversation

rdjuric commented Jul 13, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

rdjuric commented Jul 14, 2021

rdjuric commented Jul 14, 2021

marcaaron commented Jul 14, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdjuric commented Jul 15, 2021

rdjuric commented Jul 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdjuric commented Jul 19, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron commented Jul 19, 2021

OSBotify commented Jul 19, 2021

OSBotify commented Jul 23, 2021

OSBotify commented Jul 28, 2021

rdjuric commented Jul 13, 2021 •

edited

Loading

marcaaron Jul 14, 2021 •

edited

Loading