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

WP Stories integration - setting the base error notification ID and setDelete PendingIntent for error notification #12059

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented May 30, 2020

Builds on top of #12045

This PR sets the base error notification ID from the WPAndroid app to avoid collision with PostUploadNotifier.
Also, implements the PendingIntent loader for when an error notification is dismissed, and keeps tracks
Finally, we added the following notification types:

    STORY_SAVE_SUCCESS,
    STORY_SAVE_ERROR,
    STORY_FRAME_SAVE_SUCCESS,
    STORY_FRAME_SAVE_ERROR;

of which currently only STORY_SAVE_ERROR is used / needed at this point.

To test:

  1. setup some artificial error as we've been doing before in the stories library (use a counter so it fails only once)
  2. create a story, add some text / emoji
  3. save (tap publish)
  4. observe the error notification appears.
  5. tap retry to solve the issue
  6. observe the error notification gets properly cancelled.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@mzorz mzorz added this to the 15.2 milestone May 30, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 30, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@mzorz mzorz requested a review from jd-alexander May 30, 2020 15:00
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 30, 2020

You can test the changes on this Pull Request by downloading the APK here.

@mzorz mzorz changed the title WP Stories integration - setting the base error notification ID and setDelete PendingIntent for errror notification WP Stories integration - setting the base error notification ID and setDelete PendingIntent for error notification May 30, 2020
Base automatically changed from feature/wp-stories-part9-emoji-compat to feature/wp-stories-part8-upload-service June 19, 2020 23:10
@jd-alexander jd-alexander self-assigned this Jun 20, 2020
Copy link
Contributor

@jd-alexander jd-alexander left a comment

Choose a reason for hiding this comment

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

Thanks for these changes @mzorz I tested and I was able to reproduce the notification error. I added an Exception to the FrameSaveManager to simulate an error before an image frame was saved. LGTM 🚢

@jd-alexander jd-alexander merged commit 20aa71d into feature/wp-stories-part8-upload-service Jun 21, 2020
@jd-alexander jd-alexander deleted the feature/wp-stories-part92-notif-delete-intent branch June 21, 2020 00:44
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.

2 participants