Skip to content

Blogging Prompt Dashboard Card: show error when fetching prompt fails - #18548

Merged
ScoutHarris merged 2 commits into
trunkfrom
feature/18429-dashboard_card_show_error
May 10, 2022
Merged

Blogging Prompt Dashboard Card: show error when fetching prompt fails#18548
ScoutHarris merged 2 commits into
trunkfrom
feature/18429-dashboard_card_show_error

Conversation

@ScoutHarris

Copy link
Copy Markdown
Contributor

Ref: #18429, #18534 (comment)

When obtaining a prompt fails, an error message is now displayed on the dashboard prompt card.

To test:

  • Hack prompt fetching to fail - in BloggingPromptsService.fetchTodaysPrompt, call failure(nil) in the success block.
    func fetchTodaysPrompt(success: @escaping (BloggingPrompt?) -> Void,
                           failure: @escaping (Error?) -> Void) {
        fetchPrompts(from: Date(), number: 1, success: { (prompts) in
            failure(nil)
//            success(prompts.first)
        }, failure: failure)
    }
  • Enable Blogging Prompts feature.
  • Go to My Site Dashboard.
  • Verify the prompt card displays an error message.

prompt_error

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)
    N/A

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.

@ScoutHarris ScoutHarris added this to the 19.9 milestone May 10, 2022
@ScoutHarris ScoutHarris self-assigned this May 10, 2022
@ScoutHarris
ScoutHarris requested a review from wargcm May 10, 2022 00:57
@ScoutHarris
ScoutHarris marked this pull request as ready for review May 10, 2022 00:57

@wargcm wargcm left a comment

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.

Code looks good! I just have one question and a note.

  • Should the pull to refresh on the dashboard trigger the fetch prompts call?

  • I think that behavior I noted in your other PR (#18534 (comment)) can cause the card to get into a weird state. In portrait, it can have an error state but in landscape, it shows the prompt:

    error-portrait-landscape.mp4

@ScoutHarris

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @wargcm .

Should the pull to refresh on the dashboard trigger the fetch prompts call?

I assume so. However, it appears none of the cards are updated. Maybe @leandroalonso can help. I could be wrong, but it looks like pull to refresh updates the list of cards, not the cards themselves.

I think that behavior I noted in your other PR (#18534 (comment)) can cause the card to get into a weird state. In portrait, it can have an error state but in landscape, it shows the prompt:

I don't know how you forced the failure, but I couldn't repro that state. However, I did notice that I forgot to set didFailLoadingPrompt = false on success. That's now done. Maybe that was the issue?

@wargcm

wargcm commented May 10, 2022

Copy link
Copy Markdown
Contributor

@ScoutHarris

I don't know how you forced the failure, but I couldn't repro that state. However, I did notice that I forgot to set didFailLoadingPrompt = false on success. That's now done. Maybe that was the issue?

I turned off my internet on the initial load, turned it back on, and then went into landscape.

@leandroalonso

Copy link
Copy Markdown
Contributor

I assume so. However, it appears none of the cards are updated. Maybe @leandroalonso can help. I could be wrong, but it looks like pull to refresh updates the list of cards, not the cards themselves.

I haven't looked deep into that but the collection view that displays the cards is powered by a diffable data source (you can check PostsCardViewModel).

When updating the cards, the OS looks at every DashboardCardModel object and if any of these values changed from one snapshot to the other, the cell (card) is updated. If they're the same, it usually stays the same (well, sometimes it updates even by being the same but I'll leave that conversation for another time 😅).

Maybe that's why the card is not updating?

@wpmobilebot

Copy link
Copy Markdown
Contributor
You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr18548-47eba2b on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot

Copy link
Copy Markdown
Contributor
You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr18548-47eba2b on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@ScoutHarris

Copy link
Copy Markdown
Contributor Author

Thanks @leandroalonso ! I missed that whole snapshot business. 😄

@wargcm since that has nothing to do with this PR, it can be investigated separately.

@ScoutHarris

Copy link
Copy Markdown
Contributor Author

@wargcm

I turned off my internet on the initial load, turned it back on, and then went into landscape.

Thanks for the info. My latest changes seems to have fixed it, so I'm calling it good.

@ScoutHarris
ScoutHarris merged commit afd1d0c into trunk May 10, 2022
@ScoutHarris
ScoutHarris deleted the feature/18429-dashboard_card_show_error branch May 10, 2022 19:03
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