Make tapping on a Zendesk notification show Zendesk tickets list #8054
Conversation
|
From @oguzkocer
|
Just verified so on my end 👍
extra tags are set to
This is untouched by this particular PR, but good to make sure about 👍 |
| @@ -77,6 +77,10 @@ class HelpActivity : AppCompatActivity() { | |||
| } | |||
| AnalyticsTracker.track(Stat.SUPPORT_IDENTITY_FORM_VIEWED) | |||
| } | |||
|
|
|||
| if (originFromExtras == Origin.ZENDESK_NOTIFICATION) { | |||
There was a problem hiding this comment.
This check is not enough. If the activity is ever recreated, for example due to a config change such as rotation, it'll open the My Tickets again. Here are the testing instructions to replicate it:
- Tap on the Zendesk notification to open My Tickets
- Go back to HelpActivity
- Rotate the device
- Verify that My Tickets is reopened.
| @@ -416,6 +416,11 @@ public static void viewHelpAndSupport(@NonNull Context context, @NonNull Origin | |||
| context.startActivity(HelpActivity.createIntent(context, origin, selectedSite, extraSupportTags)); | |||
| } | |||
|
|
|||
| public static void viewZendeskTickets(@NonNull Context context, | |||
| @Nullable SiteModel selectedSite, @Nullable List<String> extraSupportTags) { | |||
There was a problem hiding this comment.
extraSupportTags are not necessary for push notifications. If that ever changes we can then pass it, no need to pass null to it right now.
| @@ -299,6 +305,23 @@ private void handleOpenPageIntent(Intent intent) { | |||
| } | |||
| } | |||
|
|
|||
| private void launchZendeskMyTickets() { | |||
| if (isFinishing() || getIntent() == null) { | |||
There was a problem hiding this comment.
If the intent is not used in the method, we shouldn't be checking for it. Although I can't think of one, there might be exceptions to it in which case we should write a comment for why we are checking the intent.
For this specific case, we already check that the intent is not null and the Zendesk's My Tickets should be open before ever calling the launchZendeskMyTickets method, so it's unnecessary.
Fwiw, the alternative way that would check the intent and do all of this would be returning a boolean for whether the Zendesk My Tickets will actually open. In that case we'd actually utilize the getIntent, so my initial comment would still hold. This was just given as an example, I don't recommend actually doing it, because the current code is definitely better.
There was a problem hiding this comment.
thanks for the thorough explanation - went ahead and made the change in a97bd71
| // init selected site, this is the same as in | ||
| initSelectedSite(); | ||
|
|
||
| ActivityLauncher.viewZendeskTickets( |
There was a problem hiding this comment.
Nitpick: This is way less than 120 chars, I don't think you need to separate the parameters in different lines. Even when it's done it's generally recommended to separate each one, although we have not always been able to follow that pattern.
There was a problem hiding this comment.
thanks - addressed in 76d8cff
|
I've added some single comments as I am no longer the main reviewer of this PR. My vote would be NOT to merge this into the IF, this PR is merged in, we need to change the copy of the Push Notification message because it no longer makes sense to say To expand a bit on the type of scenarios this will fail in, consider the cases where the Zendesk identity is altered. Since an identity change will reset the user's tickets, the notification will end in an empty ticket list. Whether that's better than landing them into the Me page is a question that I don't know the answer for. I was hoping we could figure that out once proper time is spent on deep linking the tickets and what kind of information we can get from Zendesk at that time. Maybe the deep linking would fail with an error we can use to communicate with the user, I am not sure. |
There is whole bunch of other tags that are added before the ticket creation: https://github.com/wordpress-mobile/WordPress-Android/blob/develop/WordPress/src/main/java/org/wordpress/android/support/ZendeskHelper.kt#L412-L437 Extra tags only handle a special circumstance.
Zendesk is an interconnected flow and the configuration build up relies on some information being there at the time of visiting the My Tickets page. Since the app might be launched from a push notification tap, we need to ensure that everything that the Zendesk configuration relies on is present. That's why that test instruction is given. |
I think the wording is still valid and makes sense. On the contrary, the expectation that the user would know and identify the text "message from Help & Support" with the equally named section of the app was a weak link / hint. It's now more contextual and evident as we lead them to the tickets list. That said, I don't have a strong feeling about changing it or leaving as is. Let me know and I'll change it to
I understand your concern - yet the current snackbar solution that shipped in 10.4 is already quite poor. Also I agree with you this is yet not the ideal (deep linking is ideal) however this is still better than the snackbar, and furthermore is quite good as a fallback to deep-linking (this is what we generally do in Notes: when we can't find the specific notification we show the list). So if nothing else, it's still a step in the right direction. I don't quite see how landing users on the "Me" tab would prevent the possible scenarios where this would fail from happening (and that can for sure continue to be investigated separately) but: even if such identity loss happens, it still is better to run the risk of that happening for some users / use cases / situations than making sure the 100% of users land on the "Me" tab. That said, I'm with you deep linking is the way to go, and I'd love to take a stab at it as a separate PR, or get to chat with you about how far the experiments went, and what has been tried and what not, to take it from there.
We're not skipping it. z.B provided we get this one merged today, we'll have still 1 week in alpha/beta testing for 10.5. |
|
@oguzkocer if you're still up for it, ready for another round 🙇 thanks in advance |
| mTicketsBeingShown = savedInstanceState.getBoolean(HelpActivity.MY_TICKETS_SHOWN_KEY) | ||
| } | ||
|
|
||
| if (originFromExtras == Origin.ZENDESK_NOTIFICATION && !mTicketsBeingShown) { |
There was a problem hiding this comment.
It might be just me, but I find this solution unnecessarily complicated and a little bit confusing. I do understand what's going on, but not everyone would be familiar with what this is solving, at least not without comments. May I suggest updating the solution as such:
diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/HelpActivity.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/HelpActivity.kt
index 787779cc2b..a7d2f38ee6 100644
--- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/HelpActivity.kt
+++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/HelpActivity.kt
@@ -38,8 +38,6 @@ class HelpActivity : AppCompatActivity() {
intent.extras?.get(WordPress.SITE) as SiteModel?
}
- private var mTicketsBeingShown: Boolean = false
-
override fun attachBaseContext(newBase: Context) {
super.attachBaseContext(LocaleManager.setLocale(newBase))
}
@@ -80,12 +78,13 @@ class HelpActivity : AppCompatActivity() {
AnalyticsTracker.track(Stat.SUPPORT_IDENTITY_FORM_VIEWED)
}
- if (savedInstanceState != null) {
- mTicketsBeingShown = savedInstanceState.getBoolean(HelpActivity.MY_TICKETS_SHOWN_KEY)
- }
-
- if (originFromExtras == Origin.ZENDESK_NOTIFICATION && !mTicketsBeingShown) {
- mTicketsBeingShown = true
+ /**
+ * If the user taps on a Zendesk notification, we want to show them the `My Tickets` page. However, this
+ * should only be triggered when the activity is first created, otherwise if the user comes back from
+ * `My Tickets` and rotates the screen (or triggers the activity re-creation in any other way) it'll navigate
+ * them to `My Tickets` again since the `originFromExtras` will still be [Origin.ZENDESK_NOTIFICATION].
+ */
+ if (savedInstanceState == null && originFromExtras == Origin.ZENDESK_NOTIFICATION) {
showZendeskTickets()
}
}
@@ -96,11 +95,6 @@ class HelpActivity : AppCompatActivity() {
refreshContactEmailText()
}
- override fun onSaveInstanceState(outState: Bundle?) {
- outState?.putBoolean(HelpActivity.MY_TICKETS_SHOWN_KEY, mTicketsBeingShown)
- super.onSaveInstanceState(outState)
- }
-
override fun onOptionsItemSelected(item: MenuItem): Boolean {
if (item.itemId == android.R.id.home) {
onBackPressed()
@@ -166,7 +160,6 @@ class HelpActivity : AppCompatActivity() {
companion object {
private const val ORIGIN_KEY = "ORIGIN_KEY"
private const val EXTRA_TAGS_KEY = "EXTRA_TAGS_KEY"
- private const val MY_TICKETS_SHOWN_KEY = "MY_TICKETS_SHOWN_KEY"
@JvmStatic
fun createIntent(
This greatly simplifies the solution by checking the nullity of savedInstanceState which tells us whether the activity is created for the first time or not. It also adds a comment explaining the process. I did double check that savedInstanceState is not null after re-creation even if we don't override the onSaveInstanceState and tested the steps here.
There was a problem hiding this comment.
💯 addressed (by directly stealing your code) in 57b7759
| @@ -38,6 +38,8 @@ class HelpActivity : AppCompatActivity() { | |||
| intent.extras?.get(WordPress.SITE) as SiteModel? | |||
| } | |||
|
|
|||
| private var mTicketsBeingShown: Boolean = false | |||
There was a problem hiding this comment.
I already suggested a different solution that'd remove this line, but just to be through and in case that change doesn't happen, may I suggest dropping the m prefix from the property and renaming it to ticketsBeingShown. m prefix is a Java specific code style that's not used in Kotlin code.
There was a problem hiding this comment.
oops! thanks for pointing that out - and agreed your suggested solution is far clearer. Thanks for putting it together for me, going with it
|
Thanks once more @oguzkocer ! Just addressed the last comments - feel free to hit the |
NOTE: This PR is an alternate (and better) approach to that of #8043 and is preferred*
Copying the description of the problem here, for completeness of this PR:
This PR makes tapping on the Zendesk notification bring the "My Tickets" screen to the foreground, in order to complete the desired action (to look into an HE answer to a question they initiated through
Help & Support -> Contact Ussection of the app).BEFORE:
AFTER:
To test:
Metab, then tapHelp & Supportand finallyContact us.backtwice).My Ticketsscreen and loads