wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.94k stars 1.31k forks source link

Whitelisted IP addresses pref doesn't work in Jetpack settings #13407

Open malinajirka opened 3 years ago

malinajirka commented 3 years ago

Expected behavior

When I click on "Whitelisted IP addresses" in Jetpack Security settings I expect "Whitelisted IP Addresses" screen to show up.

Actual behavior

When I click on "Whitelisted IP addresses" in Jetpack Security settings nothing happens.

~Note 1: The theme issue is fixed in this PR.~ ~Note 2: This issue is blocking https://github.com/wordpress-mobile/WordPress-Android/pull/13402 and https://github.com/wordpress-mobile/WordPress-Android/pull/13408~ Note 3: It seems the logic was actually never even implemented. It works fine when the Jetpack Security settings are accessed from My Site -> Settings -> Security -> Whitelisted IP Addresses.

Steps to reproduce the behavior

Prerequisites - jetpack site

  1. Create a post on the web with a blog which is not supported on mobile
  2. Open My Site -> Settings -> (Jetpack) Security -> disable "Allow WordPress.com login"
  3. Go to My Site -> Blog Posts -> open the post from step 1
  4. Click on the question mark on the unsupported block
  5. Click on "Open Jetpack Security Settings"
  6. Click on "Whitelisted IP Addresses"
  7. Notice nothing happens

    Update 11/24 When https://github.com/wordpress-mobile/WordPress-Android/pull/13402 is merged this issue won't be reproducible anymore. The row was removed in this PR. Goal of this task is to re-add that option. (it's still available when the settings are accessed from My Site-> Site Settings -> Security)

Prerequisites - jetpack site

  1. My Site
  2. Click on "Jetpack settings" option
  3. Notice the "whitelisted IP addresses" option is not shown
malinajirka commented 3 years ago

We currently have two copies of the Jetpack Security Settings. One is within SiteSettingsFragment and the other one is in JetpackSecuritySettingsFragment. This inconsistency was introduced in this PR. Having two copies makes the code hard to maintain and very error prone since we always need to make sure to update both places.

I tried to solve this issue by redirecting the user to the JetpackSecuritySettingsFragment even from Site Settings and removing all the related code from the SiteSettingsFragment (PR). This approach worked well, unfortunately while testing the PR I realized "Whitelisted IP Addresses" row doesn't do anything on click so I created this issue.

Unfortunately, it seems this feature was never implemented in the JetpackSecuritySettingsFragment and the implementation isn't trivial.

I also tried to remove JetpackSecuritySettingsFragment and pass a flag in Intent's extras to the SiteSettings fragment. This flag would indicate that we want to open the "jetpack security" sub-screen. However, android.Preferences don't allow simulating user clicks ("performClick" method is private) and I haven't found a way how to show the sub-preference programatically.

I decided to remove the "Whitelisted IP Addresses" row from the JetpackSecuritySettingsFragment (PR) until this issue is fixed.

malinajirka commented 3 years ago

Another possible approach which would allow us to have only a single copy of the Jetpack settings:

  1. Remove JetpackSecuritySettingsFragment and activity
  2. Pass "show_jetpack_settings" flag to SiteSettingsFragment
  3. When the flag is present, remove all settings which are not Jetpack Settings It'd look like this:

    Downside of this approach is that the user would still need to click on the "Security" row to see the actual Jetpack Security settings screen or we'd need to re-structure the settings (or perhaps start adding the jetpack settings programatically).

stale[bot] commented 2 years ago

This issue has been marked as stale because:

Please comment with an update if you believe this issue is still valid or if it can be closed. This issue will also be reviewed for validity and priority during regularly scheduled triage sessions.

dangermattic commented 3 weeks ago

Thanks for reporting! 👍