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

Display login notification + Notification channel config #217

Merged
merged 7 commits into from
Jun 1, 2018

Conversation

aforcier
Copy link
Contributor

Closes #113, enabling the notification shown during login when the app is dismissed (while logging in with username/password). Also fixes a crash when doing so on Android 8.1, which is what made this a more urgent issue than it previously was.

This PR also imports all the latest changes to login library from WPAndroid, which has no behavior changes for us except for the Android 8.0 notification channel support (for reference, this was originally added to the login library in wordpress-mobile/WordPress-Android#7593).

Notification channel concerns

To get this to work, I had to create our first notification channel. These are quite 'sticky', in the sense that you can't easily modify or configure notification channels once they're created.

(more on Android 8.0 notification channels)

With this PR, the notification settings screen for the app (in the system settings) looks like this:

woo-general-notification-channel

This is aligned with WPAndroid, which also has a 'General' as well as an 'Important' channel. By doing this though, we're betting that we'll keep that channel around, more or less.

If we did decide to remove the channel (and reassign the login notification to a different channel), we can totally do that, but a section will appear in the notification settings (it's cleared if the app data is reset, though):

woo-notifications-deleted-channel

I believe on earlier Android versions the actual deleted channel names are shown explicitly.

It's worth noting that any changes would presumably be figured out before the MVLP release, so this would only affect beta users who upgraded and never cleared app data.

@folletto what do you make of this approach? We could also opt for a 'Login' channel instead of 'General' if we think we're likely not to want a General channel in the long run.

I'm marking this as not ready for review in the meantime.

d3c0fa7 Update default FluxC hash
710b336 Remove unused setHelpContext from the login listeners
1fcd941 Updating to gradle 3.1.2
709e8dc Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into feature/awesome-accessibility-color-audit
e97748c Merge pull request #7656 from wordpress-mobile/feature/awesome-accessibility-lint
e20862a Merge branch 'develop' into feature/jetpack-connect-signup
cd895d4 Update WordPress Login Flow WordPress Utils library dependency to 1.20.3
e27a027 Update WordPress Login Flow support library dependencies to 27.1.1
c9d5806 Merge branch 'develop' into feature/jetpack-connect-signup
5c0788e Merge pull request #7614 from wordpress-mobile/issue/7540-followed-blog-notifications
dc94cf9 Merge branch 'develop' into feature/jetpack-connect-signup
d50538e Merge branch 'develop' into feature/style-rule-updates
398eb4d Add null check for Jetpack connect source to magic link classes
8069afc Add Jetpack connect flag and source variables to magic link classes
831084a Bump Gradle plugin to 3.1.1
c5bb0a5 Update WordPress-Android-Lint
d9d97dc Merge branch 'develop' into issue/7540-followed-blog-notifications
546ef39 Remove unnecessary warning suppressions for synthetic accessor methods from login email fragment
72b79dd Update secondary button for Jetpack signup flow in login email fragment
cb37b15 Add start signup method to login listener interface
c67f269 Add signup sheet tracking to login analytics listener interface
4067ab1 Merge pull request #7638 from wordpress-mobile/issue/7635-update-signup-analytics
5e02945 Fix new checkstyle violations
3944fae Update IDEA style config files
3d1d5e5 Merge commit '8d78020e2eebd3ad8e7e73a8dd5f43e40ec74b38' into merge-login
983c957 Merge branch 'develop' into issue/7540-followed-blog-notifications
d70bda7 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into feature/awesome-accessibility-color-audit
bb3840a Add signup magic link opened method to login analytics listener interface
9746300 Remove signup social 2FA needed login analytics listener interface method
c6f728f Update open email client method in login listener interface
2c5bdd7 Add signup magic link open email client button clicked method to login analytics listener interface
5232ef7 bumped WordPressUtils library version to 1.20.2
8f4f5c2 Add followed sites notification subscriptions fetching on login
670d4e8 Updated login color contrast.
92bcc3c Merge branch 'develop' into issue/7505-api-26-and-multiwindow
b5599ef Merge pull request #7593 from wordpress-mobile/issue/5530-basic-notification-channels-android-o
9cf1982 updated the WordPressLoginFlow library to use the latest NofiticationCompat.Builder(context, channelID) constructor, and made changes to make sure the Notifications Channel IDs used in the hosting app are available to the library seamlessly
24b88ce bumped play services lib version
84659ab Targeting API 26 for future version of chrome os support

git-subtree-dir: libs/login
git-subtree-split: d3c0fa7
…update-login-library

# Conflicts:
#	libs/login/WordPressLoginFlow/build.gradle
#	libs/login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginMagicLinkRequestFragment.java
#	libs/login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/SignupMagicLinkFragment.java
#	libs/login/WordPressLoginFlow/src/main/res/values/styles.xml
@aforcier aforcier added [Status] Not Ready for Review needs: design Requires design input/work from a designer. labels May 25, 2018
@aforcier aforcier added this to the External closed beta group milestone May 28, 2018
@folletto
Copy link

Thanks! Shame it crashes otherwise, but it seems using notification channels can be a good thing in general.

As for the approach (btw, thanks for the detailed explanation) I think we can sensibly start with General, and then split out later from general as we get more feedback about the app and we start adding notifications. I guess there is going to be always "something" in general.

Plus, if it's just a matter of clearing app data is not nice, but it's also ok in the long term even if we decide to rename everything. :)

Also, assign the login notification (shown when dismissing the
app while logging in with username/password) to the General
channel.
@aforcier aforcier force-pushed the feature/113-login-notification branch from d020426 to 4944af5 Compare May 29, 2018 12:31
@aforcier
Copy link
Contributor Author

Thanks @folletto!

I confirmed that the behavior we discussed of changing assigned channels works (it seemed like it should, but you never know):

  1. Create 'General' channel, assign login notification to it, run app
  2. Add 'Login' channel (keeping 'General' channel), assign login notification to 'Login' channel, run app
  3. Login notification works, and notification settings has both 'General' and 'Login'
  4. (Bonus) Disable 'General' notifications in settings, login - Login notification is still shown 👍

@aforcier aforcier removed needs: design Requires design input/work from a designer. [Status] Not Ready for Review labels May 29, 2018
@aforcier
Copy link
Contributor Author

Now ready for review @AmandaRiu!

@AmandaRiu AmandaRiu self-assigned this Jun 1, 2018
@AmandaRiu AmandaRiu self-requested a review June 1, 2018 00:22
Copy link
Contributor

@AmandaRiu AmandaRiu left a comment

Choose a reason for hiding this comment

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

@aforcier I know it's not directly related to this PR, bu I added a comment regarding the WordPressLoginFlow targetSdkVersion setting. Also not directly related to this PR I opened a ticket for a minor memory leak I found in LoginBaseFormFragment while testing.

Everything else looks great 👌 I tested across the following devices:

  • Emulator API 19
  • Emulator API P
  • LG G5 API 24
  • Pixel 2 API 27

:shipit:

@@ -22,7 +22,7 @@ android {

defaultConfig {
minSdkVersion 16
targetSdkVersion 25
targetSdkVersion 26
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to verify: Are we intentionally not targeting 27? For what it's worth I changed this to 27 and tested on APIs: 19, 24, 27, and P. All good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only 26 because it's coming from the WordPress app and that's still targeting 26. I believe this is ignored and we're actually targeting 27 for the overall Woo app, so it's probably okay to bump it to 27, but we'd have to be careful not to push that upstream and cause issues for the WordPress app, so I ignored it.

@AmandaRiu AmandaRiu merged commit b24b9e7 into develop Jun 1, 2018
@AmandaRiu AmandaRiu deleted the feature/113-login-notification branch June 1, 2018 06:32
jtreanor added a commit that referenced this pull request Mar 7, 2019
5ce9b47 Merge pull request #8 from wordpress-mobile/circleci
f3cad76 Removed Travis CI config
db2d8db Add CircleCI config
4896a91 Merge pull request #6 from wordpress-mobile/merge-wca
16a871b Set base button color to white
d6e2bf4 Update login library default FluxC hash
7b3ba9e Merge commit 'e500cd63388ba77e7ea13df2fbb199c64e7107c2' into feature/import-latest-login-lib
42eb5f7 Set image content description or mark it as not important for accessibility
916621c Support setting the scheme in LoginMagicLinkRequestFragment
7f9fca9 Update gradle plugin to 3.1.3
2520aa3 Update google repository config and list it first
853823c Merge pull request #217 from woocommerce/feature/113-login-notification
848294d Use more generic naming for the login notification channel id
fa437d3 Merge commit '09a0852581dd9a3a6b15109f3a8e9a80e644fbbb' into feature/update-login-library
6b07a92 Enable Google login
066bede Update gradle plugin to 3.1.2
3e1abf1 Fix FluxC build error in login library
37707a7 Merge pull request #188 from woocommerce/feature/gradle-3.1.1
1a93319 Update gradle plugin and support libraries to latest
9af7502 Update inner import checkstyle violations in login library
d07401c Update IDEA style config files
ad4fc3b Revert unintentional color change in the WordPressLoginFlow module.
c1b5671 Merge branch 'feature/order-details-screen' into feature/order-detail-views
6c93c92 Exclude utils library from FluxC imports
3727190 Merge branch 'develop' into feature/order-detail-views
eec2898 Restore login notification styling changes
103dd8f Merge commit '6bd01d64a2d01176f5b26ae36836d4f5ff0fcdc9' into feature/update-login-lib
97462cc Merge branch 'develop' into feature/order-detail-views
20e9f3b Move login icon colors to variables
b15f663 Merge branch 'develop' into feature/114-login-style-refactor
26d1088 Fix style violations in login library
17326be Order Details - Customer Info View - Action Icons and listeners, updated icon assets * Add the ability to click to dial customer phone * Add the ability to click to email customer * Add the ability to click to sms customer
ebfa842 Make login notification styling overridable
5494d0e Make login toolbar styling overridable
8cbdb5b Use overridable color names in login flow styles
e9137fe Replace WordPress buttons with LoginTheme
bf34dab Expose play-services-auth dependency from login library
1920389 Merge commit '9751124a62caad5f7c9af772f8b872b604cd02b9' into feature/gradle-3.0
f5b58af Add WPCOM_LOGIN_ONLY LoginMode, disallowing self-hosted login

git-subtree-dir: libs/login
git-subtree-split: 5ce9b47
aforcier added a commit that referenced this pull request Mar 11, 2019
33d3a31 Merge pull request #10 from wordpress-mobile/merge-wpa
b3d3bc5 Update style and lint configs
752425f Update Gradle wrapper version
3bbe173 Merge commit 'e3c231d50ec6bd985404c7aec05502864b4c08d4' into subtree-updates-v3
afcb169 Merge pull request #9 from wordpress-mobile/circleci-updates
dc431ca CircleCI: Add checkstyle and assembleDebug
5ce9b47 Merge pull request #8 from wordpress-mobile/circleci
f3cad76 Removed Travis CI config
db2d8db Add CircleCI config
e81fa90 Merge pull request #9215 from wordpress-mobile/feature/update-support-lib-28
c6332e5 Move default login input row values from layout to class
2ec1551 Add icon drawable tint color assignment to login input row class
f737b09 Add icon drawable tint color to login input row attributes
8dd430c Revert update of targetSdkVersion
f50df67 Update supportLib version to 28.0.0
ac2e27a Update to Gradle 4.10.3/Android Gradle plugin 3.2.1
64e1ef8 Reorder repositories in build.gradle to fix Gradle
4896a91 Merge pull request #6 from wordpress-mobile/merge-wca
16a871b Set base button color to white
d6e2bf4 Update login library default FluxC hash
7b3ba9e Merge commit 'e500cd63388ba77e7ea13df2fbb199c64e7107c2' into feature/import-latest-login-lib
42eb5f7 Set image content description or mark it as not important for accessibility
916621c Support setting the scheme in LoginMagicLinkRequestFragment
7f9fca9 Update gradle plugin to 3.1.3
2520aa3 Update google repository config and list it first
853823c Merge pull request #217 from woocommerce/feature/113-login-notification
848294d Use more generic naming for the login notification channel id
fa437d3 Merge commit '09a0852581dd9a3a6b15109f3a8e9a80e644fbbb' into feature/update-login-library
6b07a92 Enable Google login
066bede Update gradle plugin to 3.1.2
3e1abf1 Fix FluxC build error in login library
37707a7 Merge pull request #188 from woocommerce/feature/gradle-3.1.1
1a93319 Update gradle plugin and support libraries to latest
9af7502 Update inner import checkstyle violations in login library
d07401c Update IDEA style config files
ad4fc3b Revert unintentional color change in the WordPressLoginFlow module.
c1b5671 Merge branch 'feature/order-details-screen' into feature/order-detail-views
6c93c92 Exclude utils library from FluxC imports
3727190 Merge branch 'develop' into feature/order-detail-views
eec2898 Restore login notification styling changes
103dd8f Merge commit '6bd01d64a2d01176f5b26ae36836d4f5ff0fcdc9' into feature/update-login-lib
97462cc Merge branch 'develop' into feature/order-detail-views
20e9f3b Move login icon colors to variables
b15f663 Merge branch 'develop' into feature/114-login-style-refactor
26d1088 Fix style violations in login library
17326be Order Details - Customer Info View - Action Icons and listeners, updated icon assets * Add the ability to click to dial customer phone * Add the ability to click to email customer * Add the ability to click to sms customer
ebfa842 Make login notification styling overridable
5494d0e Make login toolbar styling overridable
8cbdb5b Use overridable color names in login flow styles
e9137fe Replace WordPress buttons with LoginTheme
bf34dab Expose play-services-auth dependency from login library
1920389 Merge commit '9751124a62caad5f7c9af772f8b872b604cd02b9' into feature/gradle-3.0
f5b58af Add WPCOM_LOGIN_ONLY LoginMode, disallowing self-hosted login

git-subtree-dir: libs/login
git-subtree-split: 33d3a31
AnirudhBhat pushed a commit that referenced this pull request Aug 11, 2021
Display login notification + Notification channel config
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.

Display Login Notifications when app is backgrounded
3 participants