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

Make app more resilient to empty post-formats. #11832

Merged
merged 3 commits into from
May 6, 2020

Conversation

develric
Copy link
Contributor

@develric develric commented May 6, 2020

Fixes #11662 (and sorry, I ended up making a bit long description 🙃)

This PR is a companion to the real solution that is in the wordpress-mobile/WordPress-FluxC-Android#1575

Was able to reproduce the issue (with 14.5 and current develop) in a JN self-hosted site both adding it to the app as a JetPack or by site address.

I think the issue is due to the fact that the post-formats we support (and I'm not talking about the post types) are not meant to be customized but with some hack like this you can get them renamed and if you do not do it well you can end up sending empty strings for some of the post-formats category.

In the FluxC PR I added some check to consider not valid an empty post-format.

Notes for reviewer

  • placed in the steps a way to test using the debugger but if it comes handy I can share the JN site I already modified with the hack above for testing
  • Probably useless to say, but after FluxC PR is merged I need to point the the newly created label before this can be merged
  • For completeness, on the web the hack has UI effects with the classic editor and not with blocks but the effect on the API (with app crash) is always present
Classic Editor with custom Gallery and Aside Block Editor with standard Gallery and Aside
image image

also, we behave like the block editor (using default names) in the app Editor Post Settings but show the customized names in the Site Settings. Not sure if it's worth changing this since AFAIU post-formats customization is not a feature we support, so in my understanding to make the app more resilient to the eventual issue should be enough (let me know wdyt).

To test

Reproduce the issue

  • Compile the develop branch and install it
  • Open the FluxC project in AS and checkout the label pointed from develop (should be 1.6.11)
  • Place an Evaluate and Log breakpoint as below for self-hosted and WPCOM API relevant code
Self-Hosted XML-RPC (SiteXMLRPCClient) WPCOM API (SiteRestClient)
image image
((Map)(response = (new java.util.HashMap<String, String>()))).put("Image", "") (response.formats = new java.util.HashMap<String, String>()).put("image", "")
  • Be sure to clean the storage (should be done also in between checking self-hosted and wpcom api site) and open the app
  • While in login screen attach debugger from the fluxc project
  • Go into login flow adding the JN site by address
  • Go to Site Settings (to create the cache entry)
  • Come back to my site
  • Go again to Site Settings -> BOOM 💥-> from now on if you open the app again you should get the app continues to crash
  • Clean the app storage, restart the app attaching the debugger from fluxc, Login and select the JN site; then repeat the steps above to get the crash

Check the fix

  • Clean the app storage (or uninstall the app), repeat the steps above with the code from this PR (using the AS with the FluxC companion PR) and check the crash does not happen
  • Also check you get the following message in the Logcat
An error occurred while updating the post formats with type: INVALID_RESPONSE

Smoke test

As per title 😊, smoke test the app got from the current PR especially in Site Settings and Editor Post Settings sections.

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.

@develric develric added this to the 14.9 milestone May 6, 2020
@develric develric requested review from khaykov and planarvoid May 6, 2020 17:45
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 6, 2020

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 6, 2020

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

Copy link
Member

@khaykov khaykov 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 the fix and detailed testing instructions 👍 Works well :shipit:

@khaykov khaykov merged commit 5839af9 into develop May 6, 2020
@khaykov khaykov deleted the issue/11662-wrong-post-formats-resilience branch May 6, 2020 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants