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

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to TaxonomyXMLRPCClient (safe) #2866

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Oct 9, 2023

Parent #2798

This PR adds any missing nullability annotation to TaxonomyXMLRPCClient.java.

FYI.1: This change is safe, meaning that there shouldn't be any (major) compile-time changes associated with this change. Having said that, testing is highly recommended since the changes in this PR can affect one or more clients (ie. JP/WPAndroid, WCAndroid, etc)

FYI.2: An analysis on TaxonomyStore and its usages with our main clients suggests that this store is only being utilizing JP/WPAndroid. AFAIA WCAndroid is not using that store and as such can be safely ignored from testing.


PS: @irfano I added you as the main reviewer, not so randomly, due to the fact that you've also reviewed #2826, and I wanted you from the Jetpack/WordPress mobile team again to be aware of and sign-off on that change for JP/WPAndroid. Feel free to merge this PR directly yourself if you deem so.


Nullability Annotation List:

  1. Add missing n-a to fetch term on taxonomy xmlrpc client
  2. Add missing n-a to fetch terms on taxonomy xmlrpc client
  3. Add missing n-a to push term on taxonomy xmlrpc client
  4. Add missing n-a to delete term on taxonomy xmlrpc client
  5. Add missing n-a to terms response to terms model method
  6. Add missing n-a to term response object to term model method
  7. Add missing n-a to term model to content struct method

Nullability Checks List:

  1. Remove unnecessary is response null check
  2. Guard usages of terms response to terms model method

Warnings Resolutions List:

  1. Create missing switch cases for taxonomy xmlrpc client errors
  2. Replace value of with long parse long for string reponse

Refactor List:

  1. Reformat taxonomy xmlrpc client
  2. Replace anonymous classes with lambda

Associated Clients

It is highly recommending that this change is being tested on the below associated clients:


To Test:

  • Create a new self-hosted WordPress site for XMLRPC testing purposes (jurassic.ninja).
  • Smoke test the FluxC Example app via the TAXONOMIES screen. Verify everything is working as expected.
  • In addition to the above, using local-builds.gradle on JP/WPAndroid, smoke test any TaxonomyStore related functionality on both, the WordPress and Jetpack apps, and see if everything is working as expected. For a couple of examples, you can expand and follow the inner and more explicit test steps within:
1. Categories Settings Screen [CategoriesListFragment.kt]

ℹ️ This test applies to both, the Jetpack and WordPress app.
⚠️ It seems that this setting is not available for self-hosted WordPress sites.

  • Go to Menu sub-tab on the initial My Site screen and click on Site Settings under the
    Manage section.
  • Find the Categories under the Writing section and click on it.
  • Verify that the Categories screen is displayed and that everything works as expected.
2. Tags Settings Screen [SiteSettingsTagListActivity.kt]

ℹ️ This test applies to both, the Jetpack and WordPress app.
⚠️ It seems that this setting is not available for self-hosted WordPress sites.

  • Go to Menu sub-tab on the initial My Site screen and click on Site Settings under the
    Manage section.
  • Find the Tags under the Writing section and click on it.
  • Verify that the Tags screen is displayed and that everything works as expected.
3. Prepublishing Screens [PrepublishingTagsFragment.kt + PrepublishingCategoriesFragment.kt]

ℹ️ This test applies to both, the Jetpack and WordPress app.

  • Go to Posts screen and edit any post.
  • Click on the UPDATE button to have the bottom sheet appear (top-right).
  • Click on the Tags row and add, remove or edit any tag for this post. Click back.
  • Click on the Categories row and add, remove or create a new category for this post. Click back.
  • Click on the UPDATE NOW button to update this post (bottom).
  • Verify that this post's Tags/Categories are updated and that everything works as expected.

Error: "'switch' statement on enum type
'org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType'
misses cases: 'TIMEOUT', 'NO_CONNECTION', 'NETWORK_ERROR', 'NOT_FOUND',
'CENSORED', 'SERVER_ERROR', 'INVALID_SSL_CERTIFICATE',
'HTTP_AUTH_ERROR', 'INVALID_RESPONSE', 'NOT_AUTHENTICATED',
'PARSE_ERROR' and 'UNKNOWN'"
Warning: "Condition 'response != null' covered by subsequent condition
'response instanceof Map'"
Warning: "Redundant boxing, 'Long.parseLong()' call can be used instead"
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
FYI: 'n-a' stands for 'nullability annotations'.
Copy link
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

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

I've tested the PR on the FluxC example app and Jetpack (but not on WP to save time). I haven't noticed any issues. 🟢
I only have a comment for you to address.

@ParaskP7
Copy link
Contributor Author

👋 @irfano and thank you so much for reviewing, testing and commenting on this PR, you rock! 🙇 ❤️

FYI: I left this comment for your that hopefully clears-up this confusion on why I made that change.

@irfano irfano merged commit ee1af4f into trunk Oct 11, 2023
13 checks passed
@irfano irfano deleted the analysis/add-missing-nullability-annotations-to-taxonomyxmlrpcclient branch October 11, 2023 16:02
@ParaskP7
Copy link
Contributor Author

Thanks for approving and merging this @irfano ! 🙇 ❤️ 🚀

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.

2 participants