Implements MSC3881 (enabled and device_id fields for Pusher API) #7217
Conversation
| @@ -70,6 +70,8 @@ class PushersManager @Inject constructor( | |||
| appNameProvider.getAppName(), | |||
| activeSessionHolder.getActiveSession().sessionParams.deviceId ?: "MOBILE", | |||
| gateway, | |||
| enabled = true, | |||
| deviceId = activeSessionHolder.getActiveSession().sessionParams.deviceId ?: "MOBILE", | |||
There was a problem hiding this comment.
@ouchadam there is some obvious duplication here with the above field deviceDisplayName. It looks like it's not given us any problems using a "device id" as the display name, but the api spec expects this display name to be something more readable like "iPhone 9". I don't see that we currently get this data anywhere. What are your thoughts?
There was a problem hiding this comment.
sounds like a good change to make (assuming iOS is doing the same)
at the moment the app doesn't display the deviceDisplayName anywhere from what I can tell~
pusher deleted after taking the screenshot
There was a problem hiding this comment.
I opted to use android.os.Build.MODEL instead for deviceDisplayName. Will catch up with @gileluard on the same for parity, but I believe this is the right approach
| * Whether the pusher should actively create push notifications | ||
| */ | ||
| @Json(name = "org.matrix.msc3881.enabled") | ||
| val enabled: Boolean = false, |
There was a problem hiding this comment.
should this default to true? That would match the current behaviour when the field is omitted
There was a problem hiding this comment.
I missed this, it was only false for testing but it should be true. Good catch!
ericdecanini
requested review from
fedrunov,
a team and
mnaturel
and removed request for
ouchadam,
fedrunov and
a team
| <string name="push_gateway_item_app_id">App ID:</string> | ||
| <string name="push_gateway_item_push_key">Push Key:</string> | ||
| <string name="push_gateway_item_app_display_name">App Display Name:</string> | ||
| <string name="push_gateway_item_device_name">Device Display Name:</string> |
There was a problem hiding this comment.
Maybe we should stick to Session Display Name: and Session ID: on the UI side to keep consistency with other sreens related to sessions?
| @@ -64,14 +65,16 @@ class PushersManager @Inject constructor( | |||
| gateway: String | |||
| ) = HttpPusher( | |||
There was a problem hiding this comment.
Is it possible to set all fields with name (like you did for appId for example) to avoid any confusion?
| gateway, | ||
| lang = localeProvider.current().language, | ||
| appDisplayName = appNameProvider.getAppName(), | ||
| deviceDisplayName = AppBuildConfig.getModel(), |
There was a problem hiding this comment.
You can find the displayName of a given device in the DeviceInfo model. I created a useCase to retrieve all device info related to a given deviceId: GetDeviceFullInfoUseCase. But since PushersManager is in the core module, I don't think this is a good idea to import this use case here, since it is in a feature package. I guess we don't want import from feature into core.
Maybe you can have a new useCase in the core module GetDeviceInfoUseCase which will rely on: session.cryptoService().getMyDevicesInfoLive(deviceId).asFlow()?
And AppBuildConfig can be removed then.
There was a problem hiding this comment.
Implemented this but instead of using getMyDevicesInfoLive I opted to use getMyDevice because we need specifically only the device being used, not the other devices registered to the account
| @@ -21,9 +21,14 @@ import im.vector.app.core.utils.getApplicationLabel | |||
| import timber.log.Timber | |||
| import javax.inject.Inject | |||
|
|
|||
| class AppNameProvider @Inject constructor(private val context: Context) { | |||
| interface AppNameProvider { | |||
There was a problem hiding this comment.
Thanks for using an interface!
| pushersManager.enqueueRegisterPusher(pushKey, gateway) | ||
|
|
||
| val httpPusher = pushersService.verifyEnqueueAddHttpPusher() | ||
| httpPusher.pushkey shouldBeEqualTo pushKey |
There was a problem hiding this comment.
To ease reading, it is possible to create in the given section an object named expectedHttpPusher of type HttpPusher with all the expected values set.
Then the verification would simply be httpPusher shouldBeEqualTo expectedHttpPusher.
You can also add // Given // When // Then comments as line titles to clearly separate the sections.
There was a problem hiding this comment.
Personally I find commenting to name the sections to be not as clean as simply leaving spaces between the sections
| @@ -46,6 +50,12 @@ abstract class FlavorModule { | |||
| @Binds | |||
| abstract fun bindsFcmHelper(fcmHelper: GoogleFcmHelper): FcmHelper | |||
|
|
|||
| @Binds | |||
There was a problem hiding this comment.
I think we need to add those bindings in FDroid variant as well, no (in the dedicated FlavorModule)?
# Conflicts: # vector/src/test/java/im/vector/app/test/fakes/FakeCryptoService.kt
|
Kudos, SonarCloud Quality Gate passed! |
Type of change
Content
Implements MSC3881
Adds
enabledanddevice_idfields to the Pusher object from the new version of the API (still in unstable channels)Motivation and context
Epic: https://element-io.atlassian.net/browse/PSG-595
Closes: https://element-io.atlassian.net/browse/PSG-766
Screenshots / GIFs
N/A
Tests
Do not merge. This PR will be kept as a draft until it is ready.
Currently this is untested (both manually and written). At the time of this writing, the dev homeserver containing this MSC was unresponsive so I'm unable to test.
Unit tests are to be written and added to this PR before merging.
Please feel free to review and leave comments on the current changes
Tested devices
Checklist