Skip to content

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Nov 3, 2022

Closes: #8015

Description

In Project Salas, we've added the ability to send Just In Time Messages to all Woo Mobile users, initially displayed on the dashboard. A single message definition will be picked up both on iOS and Android. There are also cases when we want to send a message to a smaller subset of our users, or define test JITMs which are not yet ready for production. The with_query_string and without_query_string JITM rules can help here, but only if we pass useful data in the query.

JITM Query

To take advantage of these rules, this PR adds the following to the GET /jetpack/v4/jitm request, in the query part of the query object, which is passed to the JITM engine:

  • platform = ios
  • version = 11.1
  • build_type = developer for localDeveloper and CI builds. N.B. we don't include this parameter on alpha/beta/production builds.
  • device = phone or pad

Locale

There is undocumented support for a locale query parameter (top level) on our API. This is not supported in the JITM endpoints at present, but passing it now will allow us to support locale mismatch between the site and phone in future. At present, all JITMs will be localised to the site or user's default language.

locale=en_US or other phone locale code. This deliberately excludes any currency or other locale identifier information.

See pdfdoF-1Fe-p2 for full context.

Testing instructions

Configure Proxyman/Charles to view your network traffic, or use the WormHoly tools in the settings menu to view the requests later.

  1. Run the app
  2. Observe that a request is made to /jetpack/v4/jitm
  3. Observe that the request has a locale parameter with your phone's locale as the value, at the top level of the request – 1 in screenshot below
  4. Observe that the request has a query parameter, with a JSON object, with an internal query key.
  5. Observe that the value for that key has platform, version, build_type, and device values as specified above – 2 in screenshot below.

N.B. the values for these parameters, including locale, are not currently used for anything by the backend.

Screenshots

CleanShot 2022-11-03 at 14 28 53@2x


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@joshheald joshheald added type: task An internally driven task. feature: jitm Related to Just In Time Messages labels Nov 3, 2022
@joshheald joshheald added this to the 11.1 milestone Nov 3, 2022
@joshheald joshheald changed the title Issue/8015 add build identifiers to jitm request [Just In Time Messages] Add build and locale identifiers to JITM request Nov 3, 2022
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 3, 2022

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8017-0ac84df on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@joshheald joshheald marked this pull request as ready for review November 3, 2022 17:49
@joshheald joshheald requested review from koke and pachlava November 3, 2022 17:49
Copy link
Member

@koke koke left a comment

Choose a reason for hiding this comment

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

Works as expected and code looks good. However the last few commits seem more related to the JITM public release than the description of this PR. Are these actually intended for this branch?

return buildConfig == .localDeveloper || buildConfig == .alpha
case .justInTimeMessagesOnDashboard:
return buildConfig == .localDeveloper || buildConfig == .alpha
return true
Copy link
Member

Choose a reason for hiding this comment

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

Was this meant to go in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's intentional, this is the last bit required for the release of i1. I've checked that it works correctly on a release build.

Should have called it out in the description, sorry!

@joshheald
Copy link
Contributor Author

Works as expected and code looks good.

@koke thanks again for the review!

Copy link
Contributor

@pachlava pachlava left a comment

Choose a reason for hiding this comment

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

@joshheald 👋

I've checked that build and locale identifiers now show up for iPhone:

Screenshot 2022-11-04 at 13 11 06

And that they change for iPad / other locale:

SCR-20221104-inh

One small question: is it correct that when the app is not localized for the device locale in use (e.g. Ukrainian), the value still stays en_US?

Base automatically changed from issue/7916-refresh-jitms to trunk November 4, 2022 11:38
@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@joshheald
Copy link
Contributor Author

@pachlava thanks as always for the review and detailed testing!

One small question: is it correct that when the app is not localised for the device locale in use (e.g. Ukrainian), the value still stays en_US?

I'd not tested this case, but if that's the behaviour it makes sense for our use case. If the reverse were to happen, we would have a localised message in an otherwise unlocalised app, which would seem pretty strange.

I didn't code that deliberately: I use Locale.current from the system, which must return the app's locale as en_US as its fallback. Because of this, it should also support hierarchies of localisation preferences.

This adds a workaround for an apparent backend bug: only the first query item is used in the `with_query_string` rule. For that reason, we send `build_type` first, followed by `platform`, followed by alphabetical order.
@joshheald
Copy link
Contributor Author

joshheald commented Nov 4, 2022

During testing with the new JITM testing definitions, I noticed that the display of JITMs had become inconsistent. Sometimes they showed up, sometimes they didn't.

This was because the build_type parameter is ignored when it's not the first parameter in the query string. That appears to be a backend bug, and I'll follow it up to get it logged and fixed.

In the meantime, I've added a workaround in 0ac84df, by sorting the query params with build_type and platform first: the two params we are most likely to care about initially.

@joshheald joshheald enabled auto-merge November 4, 2022 12:16
@joshheald joshheald merged commit 48eacad into trunk Nov 4, 2022
@joshheald joshheald deleted the issue/8015-add-build-identifiers-to-jitm-request branch November 4, 2022 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: jitm Related to Just In Time Messages type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Just In Time Message] Add build identifiers to query string for JITMs

5 participants