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

Implements MSC3881 (enabled and device_id fields for Pusher API) #7217

Merged
merged 22 commits into from
Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/7217.wip
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implements MSC3881: Parses `enabled` and `device_id` fields from updated Pusher API
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ data class HttpPusher(
*/
val url: String,

/**
* Whether the pusher should actively create push notifications
*/
val enabled: Boolean,

/**
* The device ID of the session that registered the pusher
*/
val deviceId: String,

/**
* If true, the homeserver should add another pusher with the given pushkey and App ID in addition
* to any others with different user IDs. Otherwise, the homeserver must remove any other pushers
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ data class Pusher(
val profileTag: String? = null,
val lang: String?,
val data: PusherData,

val state: PusherState
val enabled: Boolean,
val deviceId: String?,
val state: PusherState,
) {
companion object {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ internal object PushersMapper {
profileTag = pushEntity.profileTag,
lang = pushEntity.lang,
data = PusherData(pushEntity.data?.url, pushEntity.data?.format),
state = pushEntity.state
enabled = pushEntity.enabled,
deviceId = pushEntity.deviceId,
state = pushEntity.state,
)
}

Expand All @@ -46,7 +48,9 @@ internal object PushersMapper {
deviceDisplayName = pusher.deviceDisplayName,
profileTag = pusher.profileTag,
lang = pusher.lang,
data = PusherDataEntity(pusher.data?.url, pusher.data?.format)
data = PusherDataEntity(pusher.data?.url, pusher.data?.format),
enabled = pusher.enabled,
deviceId = pusher.deviceId,
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,6 @@ package org.matrix.android.sdk.internal.database.model
import io.realm.RealmObject
import org.matrix.android.sdk.api.session.pushers.PusherState

// TODO
// at java.lang.Thread.run(Thread.java:764)
// Caused by: java.lang.IllegalArgumentException: 'value' is not a valid managed object.
// at io.realm.ProxyState.checkValidObject(ProxyState.java:213)
// at io.realm.im_vector_matrix_android_internal_database_model_PusherEntityRealmProxy
// .realmSet$data(im_vector_matrix_android_internal_database_model_PusherEntityRealmProxy.java:413)
// at org.matrix.android.sdk.internal.database.model.PusherEntity.setData(PusherEntity.kt:16)
// at org.matrix.android.sdk.internal.session.pushers.AddHttpPusherWorker$doWork$$inlined$fold$lambda$2.execute(AddHttpPusherWorker.kt:70)
// at io.realm.Realm.executeTransaction(Realm.java:1493)
internal open class PusherEntity(
var pushKey: String = "",
var kind: String? = null,
Expand All @@ -35,7 +26,9 @@ internal open class PusherEntity(
var deviceDisplayName: String? = null,
var profileTag: String? = null,
var lang: String? = null,
var data: PusherDataEntity? = null
var data: PusherDataEntity? = null,
var enabled: Boolean = true,
var deviceId: String? = null,
) : RealmObject() {
private var stateStr: String = PusherState.UNREGISTERED.name

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ internal class DefaultAddPusherTask @Inject constructor(
echo.profileTag = pusher.profileTag
echo.data?.format = pusher.data?.format
echo.data?.url = pusher.data?.url
echo.enabled = pusher.enabled
echo.deviceId = pusher.deviceId
echo.state = PusherState.REGISTERED
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ internal class DefaultPushersService @Inject constructor(
appDisplayName = appDisplayName,
deviceDisplayName = deviceDisplayName,
data = JsonPusherData(url, EVENT_ID_ONLY.takeIf { withEventIdOnly }),
append = append
append = append,
enabled = enabled,
deviceId = deviceId,
)

override suspend fun addEmailPusher(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ import java.security.InvalidParameterException
* "device_display_name": "Alice's Phone",
* "profile_tag": "xyz",
* "lang": "en-US",
* "enabled": true,
* "device_id": "abc123",
* "data": {
* "url": "https://example.com/_matrix/push/v1/notify"
* }
Expand Down Expand Up @@ -112,7 +114,19 @@ internal data class JsonPusher(
* The default is false.
*/
@Json(name = "append")
val append: Boolean? = false
val append: Boolean? = false,

/**
* Whether the pusher should actively create push notifications
*/
@Json(name = "org.matrix.msc3881.enabled")
val enabled: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

should this default to true? That would match the current behaviour when the field is omitted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed this, it was only false for testing but it should be true. Good catch!


/**
* The device_id of the session that registered the pusher
*/
@Json(name = "org.matrix.msc3881.device_id")
val deviceId: String? = null,
) {
init {
// Do some parameter checks. It's ok to throw Exception, to inform developer of the problem
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ class PushersManager @Inject constructor(
appNameProvider.getAppName(),
activeSessionHolder.getActiveSession().sessionParams.deviceId ?: "MOBILE",
gateway,
enabled = true,
deviceId = activeSessionHolder.getActiveSession().sessionParams.deviceId ?: "MOBILE",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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~

Screenshot_20220923_085600

pusher deleted after taking the screenshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

append = false,
withEventIdOnly = true
)
Expand Down