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 All Comment SqlUtils & Model Classes (breaking) #2870

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Oct 13, 2023

Parent #2799
Closes #2831

This PR adds any missing nullability annotation to all Comment related sql-util & model classes.

FYI.1: This change is breaking, meaning that any client that depended on the Comment model should update to the new APIs. As such, this change is inherently risk, meaning that there are compile-time changes associated with this change and thus needs testing to verify correctness, both on the library's and client's side.

FYI.2: Comparing to a previous such PR, in this PR there are no non-default constructors and default constructor changes. This is because of the fact that there are a couple of pseudo Comment model construction usages within the JP/WPAndroid repo (here and here), which makes it really hard to re-define and scope the Comment model. As such, and in order to avoid causing a larger refactor, adding non-default constructors constructors and deprecating the default constructor on Comment model was ignored.

PS.1: Any other changes on any other class are just some propagating missing nullability annotation changes, which stem from the main set of classes being updated, this PR's focus, plus, some other extra minor analysis, refactor and test related changes.

PS.2: As part of this 3821422 commit, the CommentsMapper.kt and its commentEntityToLegacyModel(...) function got updated to respect the updated nullability contract of the CommentModel.java model class, which is no longer allowing for a nullable mStatus, mDatePublished, mContent and mUrl fields. From my understanding, the author of CommentsMapper.kt created those mappings and the CommentEntity database entity by defaulting all non primitive fields of the Comment model to being nullable. But, from my testing, and this related PR (its response) at least 4 of them can't and shouldn't be nullable, as in some cases, if those were to be defined as nullable, that would have caused a NPE. Be extra mindful of that when reviewing this PR.


@wzieba let me know how I did on this PR, that is, in terms of updating the store/sqlutils classes and adding nullability annotations on them and their main CommentModel.java model class, in order to utilize the nullability annotations added on its fields and thus make its use null proof. If you also feel that this is a good way to proceed with such PRs, I'll then use this pattern across the board and follow a similar approach on every such PR.


Nullability Annotations List (Model):

  1. Add missing n-a to comment model

Nullability Annotations List (SqlUtils):

  1. Replace collections with cmt list sort on comments sql utils
  2. Replace anonymous classes with lambda on comment sql utils
  3. Add missing n-a to insert or update cmt on comment sql utils
  4. Add missing n-a to remove comment on comment sql utils
  5. Add missing n-a to remove comments on comment sql utils
  6. Add missing n-a to remove comment gaps on comment sql utils
  7. Add missing n-a to insert or update cmt likes on cmt sql utils
  8. Add missing n-a to get cmt likes by cmt id on cmt sql utils

Nullability Annotations List (Store):

  1. Add missing n-a to comment error on comment store
  2. Add missing n-a to on comment changed on comment store
  3. Add missing n-a to on comment likes changed on comment store
  4. Add missing n-a to get comments for site on comment store
  5. Add missing n-a to get comment by local id on comment store
  6. Add missing n-a to all payload methods on comment store

Nullability Annotations List (Utils):

  1. Add missing n-a to comment error to fetch comments payload
  2. Add missing n-a to comment error to fetched cmt likes payload
  3. Add missing n-a to comment error to push comment payload

Nullability Checks List:

  1. Guard usages of statuses on comment sql utils

Warnings Resolution List:

  1. Create missing switch branch on comment error utils
  2. Replace comments published timestamp with long compare

Warnings Suppression List:

  1. Suppress resource warning for well sql give me writable db

Refactor List:

  1. Remove unused transient level field from comment model
  2. Replace anonymous classes with lambda on comment sql utils

Associated Clients

It is highly recommending that this change is being tested alongside the below associated clients and their accompanying PRs:


To Test (REST):

  • Smoke test the FluxC Example app via the COMMENTS screen. Verify everything is working as expected.
  • In addition to the above, using local-builds.gradle on JP/WPAndroid, smoke test any CommentStore and CommentsStore 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:

To Test (XMLRPC):

  • Create a new self-hosted WordPress site for XMLRPC testing purposes (jurassic.ninja).
  • Smoke test the FluxC Example app via the COMMENTS screen. Verify everything is working as expected.
  • In addition to the above, using local-builds.gradle on JP/WPAndroid, smoke test any CommentStore and CommentsStore 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. Comment Details Screen from Comments Screen [CommentDetailFragment.java]

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

  • Go to Comments screen and tap on any comment.
  • Verify that the Comment Details screen is shown and functioning as expected.
  • For example, try replying, approving, marking as spam, liking, trashing, editing or copying a comment's link address. Additionally, make sure to verify that you can still swipe left-right to navigate between comments.
2. Comment Details Screen from Notification Tab [CommentDetailFragment.java]

ℹ️ This test applies to the Jetpack app only.

  • Go to Notifications tab, then its COMMENTS sub-tab, and tap on any comment.
  • Verify that the Comment Details screen is shown and functioning as expected.
  • For example, try replying to a comment. Additionally, make sure to verify that you can still swipe left-right to navigate between comments.
3. Edit Comment Screen from Comments Screen [UnifiedCommentsEditActivity.java]

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

  • Go to Comments screen and tap on any comment.
  • From the More on the right side, at the bottom of the comment, select Edit.
  • Verify that the Edit Comment screen is shown and functioning as expected.
  • For example, try editing/adding the comment's Name, Web address, Email address and its Comment itself.
4. Edit Comment Screen from Notification Tab [UnifiedCommentsEditActivity.java]

ℹ️ This test applies to the Jetpack app only.

  • Go to Notifications tab, then its COMMENTS sub-tab, and tap on any comment.
  • From the More on the right side, at the bottom of the comment, select Edit.
  • Verify that the Edit Comment screen is shown and functioning as expected.
  • For example, try editing/adding the comment's Name, Web address, Email address and its Comment itself.
5. Reader Comments Screen [ReaderCommentListActivity.java]

ℹ️ This test applies to the Jetpack app only.

  • Go to Reader tab, then its DISCOVER sub-tab, and tap on any post.
  • Tap on the Comments button at the bottom of the post.
  • Verify that the Reader Comments screen is shown and functioning as expected.
  • For example, try (un)following, enabling/disabling in-app notifications, replying to post, and then sharing, replying or liking a specific comment.

Merge instructions:

  1. Wait for the accompanying JP/WPAndroid#19389 to be reviewed and approved.
  2. Remove the [PR] Not Ready for Merge label.
  3. Merge this PR.
  4. Follow-up with the merge instructions on the accompanying JP/WPAndroid#19389 client PR.

This transient 'level' field doesn't seem to be used within FluxC or
any client app that might be using the this 'CommentModel' model.

FYI: There is another such definition of a transient 'level' field,
which is actually still in use, but that is on the JP/WPAndroid level
and via the 'ReaderComment' model.

PS: Actually, JP/WPAndroid is the only client that is using this
'CommentModel' model, or 'CommentStore' for that matter.
FYI: 'n-a' stands for 'nullability annotations'.

PS: This 'NotNullFieldNotInitialized' warning got suppressed because a
table related 'model' class can never have its fields initialized via a
constructor initialization, or otherwise for that matter.
FYI: 'n-a' stands for 'nullability annotations'.
Warning: "'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', 'AUTHORIZATION_REQUIRED', 'NOT_AUTHENTICATED',
'PARSE_ERROR' and 'UNKNOWN'"
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'.
FYI: 'n-a' stands for 'nullability annotations'.
Warning: "Collections.sort could be replaced with List.sort"
Warning: "Expression can be replaced with 'Long.compare'"
Warning: "'SQLiteDatabase' used without 'try'-with-resources statement"
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'.
Base automatically changed from analysis/add-missing-nullability-annotations-to-comment-clients to trunk October 18, 2023 11:37
…ndroid into analysis/add-missing-nullability-annotations-to-all-comment-sqlutils-model-classes
Copy link
Contributor

@wzieba wzieba left a comment

Choose a reason for hiding this comment

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

Solid work @ParaskP7 ! Pre-approving as I haven't found dangerous blockers. Leaving some comments for discussion though.

Related PR Comment: https://github.com/wordpress-mobile/
WordPress-FluxC-Android/pull/2870#discussion_r1363993819
@ParaskP7
Copy link
Contributor Author

👋 @wzieba and thanks for reviewing, testing and leaving all those thoughtful comment, very useful indeed! 🙇 ❤️ 🚀

FYI: I have replied to all of your comments and pushed a commit to resolve one of them. Please take a look and let me know. I'll wait merging this PR until all comments are resolved. 🙏

@wzieba
Copy link
Contributor

wzieba commented Oct 19, 2023

Thanks for answering comments and the commit, LGTM :shipit: ! @ParaskP7

@ParaskP7 ParaskP7 merged commit 5f7448d into trunk Oct 19, 2023
2 checks passed
@ParaskP7 ParaskP7 deleted the analysis/add-missing-nullability-annotations-to-all-comment-sqlutils-model-classes branch October 19, 2023 09:44
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.

Nullability Annotations to Java Classes - SqlUtils & Model - Comment
2 participants