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

Enable jetpack app features #15946

Merged
merged 32 commits into from
Feb 18, 2022
Merged

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Feb 11, 2022

Closes #15915
Depends on wordpress-mobile/WordPress-Login-Flow-Android#79

This is a feature branch PR to enable Jetpack app features.

To test:
Please follow the testing steps covered in the linked sub-tasks.

Merging Notes:

  • Wait till referenced login lib PR removing Jetpack app sites filtering is merged to trunk (I will do this merging).
  • Replace wordPressLoginVersion in build.gradle
  • Remove Not Ready for Merge label and merge the PR.

Regression Notes

  1. Potential unintended areas of impact
    Covered in individual sub-tasks.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Covered in individual sub-tasks.

  3. What automated tests I added (or what prevented me from doing so)
    Covered in individual sub-tasks.

PR submission checklist:

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 11, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 11, 2022

You can test the changes on this Pull Request by downloading the APKs:

Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

👋 @ashiagr - I ❤️ the implementation by use of config flags .. it makes it easy to test + add/remove features in the future. Good catch on the shortcuts and feature announcements. And super kudos on the login pieces (very tricky)

I was able to test the following PRs in this feature build and everything worked as expected. I ran through JP and WP.

I'm going to approve, but not merge. I would love to give it one more go after #15944 has been merged in.

Thanks for all the great work!!

@ashiagr
Copy link
Contributor Author

ashiagr commented Feb 14, 2022

Thanks so much @zwarm!

I've merged #15944 and enabled normal login flow if no sites are found (and site creation is enabled) for Jetpack app instead of showing the "No Jetpack sites" screen.
Also, if signup is enabled, Jetpack app will now show a PostSignupInterstitial screen the first time it is shown similar to the WordPress app.

Note: Illustrations on PostSignupInterstitial screen and MySite empty screen are a bit off, I can either
i) hide them or
ii) wait to replace them till we have the new ones. I'll create a task to track it.

EDIT: I've hidden MySite empty screen illustration for Jetpack app as it had WordPress app icon in 1f76982.

Ready for another look. 🙇‍♀️

@ashiagr ashiagr added Tooling and removed Tooling labels Feb 15, 2022
Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

@ashiagr - I stumbled across a few issues.

  • In Jetpack App: Upon first login to Jetpack app, the illustration in the view appears to be a WP one.

Alt desc

2. In jetpack App: Tapping on Me -> log out doesn't log me out on the first attempt. I have to do it twice. See video.
Screen_Recording_20220216-160110_Jetpack.Pre-Alpha.mp4

@zwarm zwarm assigned zwarm and ashiagr and unassigned ashiagr Feb 16, 2022
@ashiagr ashiagr added Tooling and removed Tooling labels Feb 17, 2022
# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt
#	build.gradle
@ashiagr
Copy link
Contributor Author

ashiagr commented Feb 18, 2022

👋 @zwarm,

I've replaced PostSignupInterstitial screen image (f2b93e4, df5022c):

Light Dark
lt_device-2022-02-17-161734 dark_device-2022-02-17-160859

I've also merged login lib PR and updated the corresponding commit hash in this PR. - f552f2e

Regarding the logout (twice click) issue, it is difficult to find out a root cause in a limited time frame, but given that it can be reproduced only when there are no sites for the Jetpack app, and users are able to logout on the next attempt, we can find a fix as part of a separate PR. I've created an issue here to track it.

This is now ready for another look. 🙇‍♀️

@ashiagr ashiagr requested a review from zwarm February 18, 2022 13:11
Copy link
Contributor

@zwarm zwarm 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 replaced PostSignupInterstitial screen image

I now see the Jetpack illustration when logging in for the first time with an account that has no sites. 👍

Thanks for creating that new PR for the double logout issue. I agree with you, this is definitely an edge case.

I tested the login cases again and all is working as expected. 🙇 Nice job!

@ashiagr ashiagr merged commit f315532 into trunk Feb 18, 2022
@ashiagr ashiagr deleted the feature/15915-enable-jetpack-app-features branch February 18, 2022 14:31
@zwarm zwarm mentioned this pull request May 19, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changes to Jetpack App
2 participants