Skip to content
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

My Site Dashboard: cache the response #17945

Merged
merged 4 commits into from
Feb 14, 2022

Conversation

leandroalonso
Copy link
Contributor

Part of #17873

Simulator.Screen.Recording.-.iPhone.13.-.2022-02-11.at.13.43.10.mp4

To test:

Important: make sure to enable My Site Dashboard flag and to sandbox the API.

Switching sites

  1. Open the app
  2. Tap the dashboard
  3. Switch to another site
  4. Go back to the previous site
  5. Note how the previous cards appear right away
  6. Close the app

Offline

  1. Disable your internet connection
  2. Open the app
  3. Tap Dashboard
  4. The cards should appear
  5. Switch the selected site
  6. The dashboard should load cards

Regression Notes

  1. Potential unintended areas of impact
    n/a

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    n/a

  3. What automated tests I added (or what prevented me from doing so)
    Added unit tests for the persistence logic.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Works as described!

func persist(cards: NSDictionary, for wpComID: Int) {
do {
let directory = try FileManager.default.url(for: .documentDirectory, in: .userDomainMask, appropriateFor: nil, create: false)
let fileURL = directory.appendingPathComponent("cards_\(wpComID).json")
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of defining the file path as a constant since we also use it in getCards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4d76493

@leandroalonso leandroalonso merged commit 57adae5 into trunk Feb 14, 2022
@leandroalonso leandroalonso deleted the issue/17873-persist-dashboard-response branch February 14, 2022 14:10
@wpmobilebot
Copy link
Contributor

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr17945-4d76493. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr17945-4d76493. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@pachlava
Copy link
Contributor

Was a bit slow: started testing the PR earlier today, but it got merged while I was on a lunch break 🥫

Just wanted to retroactively leave a note that I noticed no issues while testing, besides one thing, which does not contradict the PR description, but I still wanted to be sure that it's expected. On the video below, it's clearly seen that the cards content does get cached, but you can briefly see the spinner after the site switch. Only mentioning this because I noticed no spinner in the video from the PR description.

Screen.Recording.2022-02-14.at.13.42.12.mov

@leandroalonso
Copy link
Contributor Author

@pachlava you don't see the spinner on the PR description because I was offline while recording it!

But yes, this spinner will be gone in a subsequent task! Thanks for pointing that out. :)

@leandroalonso leandroalonso modified the milestones: Pending, 19.6 Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants