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.

Sign up for GitHub

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

Jump to bottom

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to Theme Model Classes (breaking) #2897

Merged
merged 14 commits into from
Dec 4, 2023
Merged

[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to Theme Model Classes (breaking) #2897

merged 14 commits into from
Dec 4, 2023

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Nov 9, 2023 •
edited by antonis
Loading

Parent #2799
Closes #2840

This PR adds any missing nullability annotation to ThemeModel.java model and all related store & sql-util classes.

FYI: This change is breaking, meaning that any client that depended on the ThemeModel.java model should update to the new APIs (non-default constructors) as the existing API (default constructor) are now deprecated. As such, this change is inherently risky, 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.

PS: 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: @antonis I added you as the main reviewer, not so randomly (it being a continuation of #2893), since I just wanted someone from the Jetpack/WordPress mobile team to be aware of and sign-off on that change. Feel free to merge this PR directly yourself if you deem so.

Nullability Annotations List (Model):

  1. Add missing nl-a to equals parameter on theme model
  2. Add missing n-a to theme model
  3. Add default and non-default constructors for theme model

Nullability Annotations List (Store):

  1. Add missing n-a to themes error on theme store
  2. Add missing n-a to on theme activated on theme store
  3. Add missing n-a to on theme removed on theme store
  4. Add missing n-a to on theme deleted on theme store
  5. Add missing n-a to on theme installed on theme store
  6. Add missing n-a to get wp com themes on theme store
  7. Add missing n-a to get wp com mobile fr. themes on theme store
  8. Add missing n-a to get themes for site on theme store

Warnings Suppression List:

  1. Suppress condition covered by further condition warning

Associated Clients

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

To Test (REST):

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

N/A

1. [JP/WP] Themes Screen [ThemeBrowserActivity.java + ThemeBrowserFragment.kt]

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

  • Go to Themes screen, verify it is shown and functioning as expected.
  • For example try scrolling up-and-down and searching for a theme.
  • Check your current theme and tap on the Customize, Details and Support buttons. Verify that everything works as expected.
  • From the themes list, find your current theme on top, click on its menu items and again tap on the Customize, Details and Support buttons. Verify that everything works as expected.
  • From the themes list, find another theme, other than your current one, click on its menu items and again tap on the Details and Support buttons. Verify that everything works as expected.
  • Then, tap on the View and Try & Customize buttons. Verify that everything works as expected.
  • Finally, tap on Activate button. Verify that the selected theme is activated and that everything works as expected.
2. [JP] Site Creation Screens [SiteCreationActivity.kt + HomePagePickerFragment.kt + DesignPreviewFragment.kt]

ℹ️ This test applies to the Jetpack app only.

  • From Choose Site, tap on the + button on top and via the Create WordPress.com site, go to Site Creation screen, verify it is shown and functioning as expected.
  • For example, while on the What's your website about? screen, select a topic from the list of topics (ie. Food).
  • Then, while on the Choose a theme screen, select a theme from the list of themes (per category, ie About).
  • Finally, verify that the Preview screen is shown and functioning as expected.

Merge instructions:

  1. Wait for the accompanying JP/WPAndroid#19583 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#19583 client PR.

ParaskP7 added 12 commits November 9, 2023 11:36
@ParaskP7
Analysis: Add missing nl-a to equals parameter on theme model
2f9c0e6 
FYI: 'n-a' stands for 'nullable annotations'.
@ParaskP7
Analysis: Suppress condition covered by further condition warning
9dadc58 
Warning: "Condition 'other == null' covered by subsequent condition
'!(other instanceof ThemeModel)'"

FYI: One could resolve this warning, simply by removing the unnecessary
'other == null' condition, but this would change the 'equals(...)' logic
and possible introduce some kind of a regression, most liked performance
wise. Thus, it is better to ignore this atm, just by suppressing it, and
call this out of scope.
@ParaskP7
Analysis: Add missing n-a to theme model
f4f622d 
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.
@ParaskP7
Analysis: Add default and non-default constructors for theme model
a02a969 
This is done so that clients of that model, clients like JP/WPAndroid,
when trying to manually instantiating such a model (if needed), they
will always use the non-default constructor.

The empty default constructor was added because '@table' related models
need to have one such constructor. Otherwise, its 'Mapper' related
auto-generated class would fail to compile, in this case the
'ThemeModelMapper.java' class.

Also, 2 full-ish-arguments constructor were added, which should be the
preferred and goto constructor for most cases, one for a WP.coma theme,
and one for a Jetpack theme.

FYI: The trick I used to first '@deprecated' this constructor is to help
clients avoid using this constructor in the future, that is, when they
manually construct one. Suppressing the 'DeprecatedIsStillUsed' warning
was necessary because otherwise, even if every occurrence in the
codebase is being switched to the non-default constructor, the
auto-generated 'Mapper' related class will anyway use that default
constructor, thus this warning will persist no matter what.
@ParaskP7
Analysis: Add missing n-a to themes error on theme store
27d46f8 
FYI: 'n-a' stands for 'nullability annotations'
@ParaskP7
Analysis: Add missing n-a to on theme activated on theme store
67bf8c3 
FYI: 'n-a' stands for 'nullability annotations'
@ParaskP7
Analysis: Add missing n-a to on theme removed on theme store
0c52f8a 
FYI: 'n-a' stands for 'nullability annotations'
@ParaskP7
Analysis: Add missing n-a to on theme deleted on theme store
d329bd1 
FYI: 'n-a' stands for 'nullability annotations'
@ParaskP7
Analysis: Add missing n-a to on theme installed on theme store
a51329c 
FYI: 'n-a' stands for 'nullability annotations'
@ParaskP7
Analysis: Add missing n-a to get wp com themes on theme store
7334e98 
FYI: 'n-a' stands for 'nullability annotations'
@ParaskP7
Analysis: Add missing n-a to get wp com mobile fr. themes on theme store
9001e95 
FYI: 'n-a' stands for 'nullability annotations'
@ParaskP7
Analysis: Add missing n-a to get themes for site on theme store
1dfeb90 
FYI: 'n-a' stands for 'nullability annotations'
@ParaskP7 ParaskP7 marked this pull request as ready for review November 9, 2023 12:40
@antonis antonis self-requested a review November 9, 2023 12:48
@antonis
Copy link

antonis commented Nov 9, 2023

PS: @antonis I added you as the main reviewer, not so randomly (it being a continuation of #2893), since I just wanted someone from the Jetpack/WordPress mobile team to be aware of and sign-off on that change. Feel free to merge this PR directly yourself if you deem so.

Makes sense @ParaskP7 👍 Heads up that I would probably get to it after the division meetup on the week starting on November 20th.

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Nov 9, 2023

Makes sense @ParaskP7 👍 Heads up that I would probably get to it after the division meetup on the week starting on November 20th.

Thanks for the heads-up @antonis and no worries, please take your time with it. And actually, since I am on paternity leave from next week, feel free to move forward with this, and its accompanying JP/WPAndroid PR as you see fit. 😊

@ParaskP7 ParaskP7 mentioned this pull request Nov 9, 2023
Merged
16 tasks
Antonis Lilis added 2 commits December 4, 2023 14:35
Merge branch 'trunk' into analysis/add-missing-nullability-annotation…
6a898de 
…s-to-theme-model-classes

# Conflicts:
#	fluxc/src/main/java/org/wordpress/android/fluxc/model/ThemeModel.java
#	fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpcom/theme/ThemeRestClient.java
#	fluxc/src/main/java/org/wordpress/android/fluxc/persistence/ThemeSqlUtils.java
#	fluxc/src/main/java/org/wordpress/android/fluxc/store/ThemeStore.java
Tidies up code change related to #2887
719621b
antonis
antonis approved these changes Dec 4, 2023
View reviewed changes
Copy link

@antonis antonis left a comment

Choose a reason for hiding this comment

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

Awesome work @ParaskP7 🙇
The code looks great and the app behaved as expected in my tests. I've also retested the changes introduced with #2887 after merging from trunk and fixing the conflicts 🎉

@antonis antonis merged commit b4e5100 into trunk Dec 4, 2023
13 checks passed
@antonis antonis deleted the analysis/add-missing-nullability-annotations-to-theme-model-classes branch December 4, 2023 14:45
@0nko 0nko mentioned this pull request Dec 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonis antonis antonis approved these changes

Assignees
No one assigned
Labels
Core Tech Debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nullability Annotations to Java Classes - SqlUtils & Model - Theme
2 participants
@ParaskP7 @antonis