Skip to content

Redirect to first reportID if the path is empty - #191

Merged
tgolen merged 5 commits into
masterfrom
joe-first-report
Aug 18, 2020
Merged

tgolen merged 5 commits into
masterfrom
joe-first-report

Conversation

@Jag96

@Jag96 Jag96 commented Aug 15, 2020 •

Copy link
Copy Markdown
Contributor

Fixes #83

Details

This PR ensures that when the app loads, we show the history for the first report in the LHN. This is the same change as in #132, but without the merge issues.

Tests

  1. Sign out of the web app and navigate to http://localhost:8080/#/ (no exitTo) and sign back in, confirm that you are redirected to the first chat after it loads
  2. Delete the mobile app from the simulator/emulator (manually through the UI) and then run npm run ios/android. Confirm that the hamburger menu is not open on load, and a chat automatically loads

@Jag96
Jag96 requested a review from tgolen August 15, 2020 20:37
@Jag96 Jag96 self-assigned this Aug 15, 2020
Comment thread src/lib/actions/ActionsReport.js Outdated
return new Promise(resolve => allSettledPromise
.then(() => {
fetchedReports = _.sortBy(fetchedReports, 'reportID');
resolve(fetchedReports);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We weren't actually returning the reports here, just a list of promise results and undefined values (AsyncStorage.mergeItem resolves null from the Ion.merge call), so I added a resolve to return the newReport data.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think that was on purpose. We shouldn't be returning data from the actions. The actions should call the API, and then put the data straight into Ion. Any other code that needs access to the data should be subscribing to it from Ion.

@Jag96 Jag96 Aug 16, 2020 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, that makes sense! I was going to say let's add this to the readme, but looks like you're already on top of it in #205

Comment thread src/page/HomePage/SidebarView.js Outdated
import Ion from '../../lib/Ion';
import {withRouter} from '../../lib/Router';

let urlLocationObject = {};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the best way to do this, but I had trouble getting the path variable in a way that worked for both web and mobile. On web, grabbing the location from window.location works, but not for mobile (window technically exists, but it doesn't have location or match). I tried a few other methods, but I couldn't access the SidebarView's props inside the loader callback at the bottom of the file which ultimately lead me to this.

If there's a better React-y way to do this, lmk and I'll have a look! I also think using a PubSub pattern would work here, but I'd rather avoid adding it if we don't need to.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of other options that I would suggest (in this order):

  1. Do the redirect from componentDidMount where it has access to the props
  2. Get the URL from Ion.get(IONKEYS.CURRENT_URL) (will probably have some race conditions because this value is set asynchronously from another component)
  3. Do the redirect from ActionsReports.js via a param passed to fetchAll loader method (there is a loaderParams setting for Ion binding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue I ran into with doing the redirect in componentDidMount is that it was invoked before the data was fetched, so while I could access the location, I didn't have a reportID to redirect to. Is there another way you were thinking of making that work?

I'll have a look into approach 2 and see if I can get any race conditions to happen.

Comment thread src/lib/actions/ActionsReport.js Outdated
return new Promise(resolve => allSettledPromise
.then(() => {
fetchedReports = _.sortBy(fetchedReports, 'reportID');
resolve(fetchedReports);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think that was on purpose. We shouldn't be returning data from the actions. The actions should call the API, and then put the data straight into Ion. Any other code that needs access to the data should be subscribing to it from Ion.

Comment thread src/page/HomePage/SidebarView.js Outdated
import Ion from '../../lib/Ion';
import {withRouter} from '../../lib/Router';

let urlLocationObject = {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of other options that I would suggest (in this order):

  1. Do the redirect from componentDidMount where it has access to the props
  2. Get the URL from Ion.get(IONKEYS.CURRENT_URL) (will probably have some race conditions because this value is set asynchronously from another component)
  3. Do the redirect from ActionsReports.js via a param passed to fetchAll loader method (there is a loaderParams setting for Ion binding.

@Jag96

Jag96 commented Aug 16, 2020

Copy link
Copy Markdown
Contributor Author

Updated, I couldn't reproduce any race conditions using IONKEYS.CURRENT_URL, but let me know if you see any issues.

@AndrewGable

Copy link
Copy Markdown
Contributor

FYI - Conflicts

@Jag96

Jag96 commented Aug 18, 2020

Copy link
Copy Markdown
Contributor Author

Conflicts resolved

Comment thread src/lib/actions/ActionsReport.js Outdated
return _.isEmpty(report) ? null : _.values(report)[0];
})))
.then(() => Ion.get(IONKEYS.SESSION, 'accountID'))
.then(() => Ion.set(IONKEYS.REPORT_IDS, _.pluck(fetchedReports, 'reportID')))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than storing an entire array, how about just storing the first one? Since localStorage has limited space, storing the least amount of data there is a good optimization.

Suggested change
.then(() => Ion.set(IONKEYS.REPORT_IDS, _.pluck(fetchedReports, 'reportID')))
.then(() => Ion.set(IONKEYS.REPORT_IDS, _.first(_.pluck(fetchedReports, 'reportID'))))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, I'll update the ion key to be FIRST_REPORT_ID so it isn't misleading

@tgolen
tgolen merged commit 247ea38 into master Aug 18, 2020
@tgolen
tgolen deleted the joe-first-report branch August 18, 2020 16:55
mountiny pushed a commit that referenced this pull request Feb 12, 2025
sajal101agrawal pushed a commit to sajal101agrawal/expensify-app that referenced this pull request Sep 16, 2026
Redirect to first reportID if the path is empty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

open to first room in the LHN

3 participants