WordPress-Android icon indicating copy to clipboard operation
WordPress-Android copied to clipboard

[Hack] Add logic to BuildConfig to allow for announcement debugging.

Open notandyvee opened this issue 1 year ago • 20 comments

The current announcement logic is a bit of a pain to test. Normally I'd add a feature flag. But I thought instead let's add something in build.gradle to use local.properties so it's a bit easier to test.

Now you can simply add alwaysShowAnnouncement=true to your local.properties, build the app in debug, and the announcements from your current release should be shown every time you load the app.

Thoughts?


To Test:

  • [x] Install the apk from this PR and make sure you aren't seeing an announcement. There is currently not one in this current release, 24.7-rc-1.
  • [x] Now checkout this PR and build it locally.
  • [x] You shouldn't see the announcement, unless you somehow upgraded the build from the previous version to 24.7. I created a test announcement for you for release 24.7.
  • [x] Add alwaysShowAnnouncement=true to your local.properties file and rebuild. You should be seeing the announcement pop up on app load everytime.

Extra credit:

  • [ ] Rebuild the app in release mode and leave the alwaysShowAnnouncement=true. You should not be getting the announcement. This covers us in the event it slips into production. Should be impossible considering local.properties would need also have the alwaysShowAnnouncement in CI when producing release builds.

Note: I was going to add documentation, but instead decided against it until this PR is reviewed.


Regression Notes

  1. Potential unintended areas of impact

    n/a

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    Manual test.

  3. What automated tests I added (or what prevented me from doing so)

    n/a


PR Submission Checklist:

  • [x] I have completed the Regression Notes.
  • [x] I have considered adding accessibility improvements for my changes.
  • [x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

notandyvee avatar Mar 15 '24 21:03 notandyvee

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20486-5518e1f
Commit5518e1fd9fbae7fabd6a94a55a6dd610d1301189
Direct Downloadwordpress-prototype-build-pr20486-5518e1f.apk
Note: Google Login is not supported on these builds.

wpmobilebot avatar Mar 15 '24 22:03 wpmobilebot

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20486-5518e1f
Commit5518e1fd9fbae7fabd6a94a55a6dd610d1301189
Direct Downloadjetpack-prototype-build-pr20486-5518e1f.apk
Note: Google Login is not supported on these builds.

wpmobilebot avatar Mar 15 '24 22:03 wpmobilebot

Codecov Report

Attention: Patch coverage is 45.45455% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 40.66%. Comparing base (6c352ca) to head (5518e1f).

Files Patch % Lines
.../android/viewmodel/main/WPMainActivityViewModel.kt 57.14% 0 Missing and 3 partials :warning:
.../org/wordpress/android/ui/prefs/AppPrefsWrapper.kt 33.33% 2 Missing :warning:
.../java/org/wordpress/android/ui/prefs/AppPrefs.java 0.00% 1 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #20486   +/-   ##
=======================================
  Coverage   40.66%   40.66%           
=======================================
  Files        1490     1490           
  Lines       68621    68629    +8     
  Branches    11338    11340    +2     
=======================================
+ Hits        27907    27911    +4     
- Misses      38195    38198    +3     
- Partials     2519     2520    +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 15 '24 22:03 codecov[bot]

👋 My setup wasn't working so I couldn't test. See https://a8c.slack.com/archives/C441E3YTS/p1711413127791989?thread_ts=1711405412.749429&cid=C441E3YTS for details.

Given this, I think it would be best if @irfano could review 🙏 , so I removed myself as a reviewer.

guarani avatar Mar 26 '24 13:03 guarani

@notandyvee, I am the reviewer but the PR is still draft. Is this ready for review?

irfano avatar Mar 31 '24 14:03 irfano

Apologies @irfano I just haven't had time. Let me carve out time today to make the documentation and then convert it to a final PR. Since Paul was having trouble it became clear it needs some documentation.

notandyvee avatar Apr 15 '24 16:04 notandyvee

It's ready @irfano . I held off on writing some docs until I hear what you think. Please note this PR does not need to be merged. If you feel strongly against adding this, we can close and consider a different way of accomplishing the same thing. No pressure. Adding @aditi-bhatia to the reviewers to get some more opinions.

notandyvee avatar Apr 15 '24 21:04 notandyvee

I added alwaysShowAnnouncement=true to local.properties and built jetpackWasabiDebug but couldn't see the announcement. What am I missing? 🤔

irfano avatar Apr 16 '24 15:04 irfano

I added alwaysShowAnnouncement=true to local.properties and built jetpackWasabiDebug but couldn't see the announcement. What am I missing? 🤔

I don't use wasabi. This could be another area of concern. Could you try jalapeno first to see whatsup @irfano ?

notandyvee avatar Apr 16 '24 15:04 notandyvee

Our purpose of this is to see how the announcement will look on the screen, right? I find putting a config in the Debug settings screen preferable because testers who are not Android developers can also utilize this setting. WDYT?

I noticed that iOS has some buttons for similar features on its Debug settings screen. We can do the same.

irfano avatar Apr 16 '24 15:04 irfano

I noticed that iOS has some buttons for similar features on its Debug settings screen. We can do the same.

I'll need some time to look into how iOS is doing it. Android's is a bit clunky, which is why I decided against it. But sure.

notandyvee avatar Apr 16 '24 21:04 notandyvee

We have a "FEATURES IN DEVELOPMENT" tab in the "Debug settings". Perhaps we can add a fourth tab to this screen, "LOCAL CONFIGS", and use the same design as "FEATURES IN DEVELOPMENT.

I believe being able to find everything related to testing configs within "Debug settings" would be a better option.

irfano avatar Apr 17 '24 19:04 irfano

It appeared to me on trunk build once. Is that the upgrade to 24.7 scenario showing up?

ravishanker avatar Apr 22 '24 06:04 ravishanker

We have a "FEATURES IN DEVELOPMENT" tab in the "Debug settings". Perhaps we can add a fourth tab to this screen, "LOCAL CONFIGS", and use the same design as "FEATURES IN DEVELOPMENT.

I believe being able to find everything related to testing configs within "Debug settings" would be a better option.

You could just add it to Features in development tab, and stays there forever.

ravishanker avatar Apr 22 '24 06:04 ravishanker

@irfano sorry I didn't get to this sooner. Mind taking a look when you get a chance and give me your thoughts. I did what was mentioned. Add a new tab on the debug settings screen. I decided to create a new UiItem, SharedPrefsField. This item currently only represents a boolean, but can be a string, long, etc. Hence why I am using a type. I tried to keep it simple since it's only for debugging.

notandyvee avatar May 10 '24 21:05 notandyvee

@notandyvee, while reviewing the change, I noticed a screen that already contains your new pref, pref_always_show_announcement. This screen was introduced 4 months ago (#19880). Do you think we can just add a new pref and use this existing screen?

irfano avatar May 11 '24 11:05 irfano

@notandyvee, while reviewing the change, I noticed a screen that already contains your new pref, pref_always_show_announcement. This screen was introduced 4 months ago (https://github.com/wordpress-mobile/WordPress-Android/pull/19880). Do you think we can just add a new pref and use this existing screen?

Sigh 🤦🏻‍♂️ . I do think we should re-use that screen. Just sucks we didn't catch that sooner. Gonna be the 3rd iteration. I'll look into it on monday.

notandyvee avatar May 11 '24 17:05 notandyvee

@justtwago I made some edits to DebugSharedPreferenceFlagsViewModel that require your approval. I needed a debug shared pref value for debugging. Since the shared pref needs to be written first before it's seen on that screen, I thought it made more sense to add any flags we want to test in a list of enums. This way any test ones can be toggled.

The other big reason I felt this change would be helpful is to open it up for strings and other primitive types. Maybe test urls or other text we can change for debug purposed. I didn't make that change as it's not required yet, but it's why I added KClass in the DebugPrefs enum. Thoughts?

notandyvee avatar May 13 '24 22:05 notandyvee

@irfano I'll fix the failing test.

What does "Class used to track" mean?

I started to add class doc and stopped mid way 😅 . Updated now.

Now, I can only see pres_always_show_announcement

This is on purpose. It's why I pinged @justtwago . Currently the preference flag needs to exist in order for it to show up. I just updated it to be more explicit. If you have no issues feel free to approve, but do not merge. We can wait for Arty to put his opinion.

notandyvee avatar May 14 '24 15:05 notandyvee