Fixing grouped notification causing malformed url #4320

ouchadam · 2021-10-22T15:20:43Z

We're attaching unhandled URIs to our notification data payload which the home screen then attempts to parse and fails.

These Intent.data URIs are needed because they enforce uniqueness of the intents (extras are not evaluated by Intent.filterEquals), android 29 introduces a dedicated Intent.identifier aimed at handling this case but we'll have to wait awhile until 29 becomes the minimum version.

For the time being this PR extracts the URI schema and filters it within the HomeActivity deeplink handling.

BEFORE GROUP	AFTER GROUP

BEFORE INVITE	AFTER INVITE

github-actions · 2021-10-22T15:50:00Z

Unit Test Results

  48 files ±0   48 suites ±0 48s ⏱️ -6s
  91 tests ±0   91 ✔️ ±0 0 💤 ±0 0 ❌ ±0
244 runs ±0 244 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 55c00a0. ± Comparison against base commit 09fbd5f.

♻️ This comment has been updated with latest results.

ouchadam · 2021-10-22T16:01:10Z

vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt

                call = call,
                mode = null).apply {
            flags = Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP
-            data = Uri.parse("foobar://$call.callId")


there was a small string interop error here $call.callId instead of ${call.callId}, I wonder if it could have been causing some issues with the outgoing call notification 🤔

Good catch! IDK about an issue fix here. It's OutgoingRingingCallNotification, @ganfra any idea?

Don't think it's actually used

bmarty · 2021-10-22T16:29:48Z

vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt

                    // pending intent get reused by system, this will mess up the extra params, so put unique info to avoid that
-                    contentIntent.data = Uri.parse("foobar://" + inviteNotifiableEvent.eventId)
-                    setContentIntent(PendingIntent.getActivity(context, 0, contentIntent, 0))
+                    contentIntent.makeUnique(inviteNotifiableEvent.eventId)


I think the comment above (// pending intent get reused by system, this will mess up the extra params, so put unique info to avoid that) is important.
This comes from Riot-Android
@BillCarsonFr can give his POV maybe?

👍 would be great for some more context

I've still kept the uniqueness but now its from the intent.extras rather than the intent.data since the deeplink mechanism is reading from intent.data

* Determine if two intents are the same for the purposes of intent * resolution (filtering). That is, if their action, data, type, identity, * class, and categories are the same. This does <em>not</em> compare * any extra data included in the intents. Note that technically when actually * matching against an {@link IntentFilter} the identifier is ignored, while here * it is directly compared for equality like the other fields.

which means the extra is not enough to provide uniqueness 😢

android 29 provides a way to set a unique Id directly onto the intent which would work for pending intents but we need to support earlier versions.

I'll revert these changes and instead replace foobar:// with a constant which gets filtered by the intent handling

- fixes malformed url errors appearing for uri we only create to force uniqueness in the notifications

ouchadam · 2021-10-25T08:16:49Z

vector/src/main/java/im/vector/app/features/permalink/PermalinkHandler.kt

-                handlePermalink(permalinkData, deepLink, context, navigationInterceptor, buildTask)
+        return when {
+            deepLink == null                                    -> false
+            deepLink.isIgnored()                                -> true


this is the fix, we're marking ignored deeplinks as handled to avoid attempting to handle them

Thanks, not your fault, but this is where a doc would help, to understand what returning false or true means...

I'm happy to fix in this PR or a separate one, will also use the opportunity to add some tests

No hurry, can you just fix the commented out code issue and add a changelog file so I can prepare release 1.3.5. Thanks!

bmarty

Thanks you, LGTM

bmarty · 2021-10-25T09:18:10Z

vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt

                // the action must be unique else the parameters are ignored
                quickReplyIntent.action = QUICK_LAUNCH_ACTION
-                quickReplyIntent.data = Uri.parse("foobar://$roomId")
+                quickReplyIntent.data = _root_ide_package_.im.vector.app.core.extensions.createIgnoredUri($roomId")


Strange change in this (commented out) code

🤔 totally missed that, I guess android studio didn't like auto extracting within the commented code

Yes, probably

bmarty · 2021-10-25T09:20:13Z

vector/src/main/java/im/vector/app/features/permalink/PermalinkHandler.kt

-                handlePermalink(permalinkData, deepLink, context, navigationInterceptor, buildTask)
+        return when {
+            deepLink == null                                    -> false
+            deepLink.isIgnored()                                -> true


Thanks, not your fault, but this is where a doc would help, to understand what returning false or true means...

bmarty · 2021-10-25T10:19:14Z

This PR will be merged, so I'm not requiring new changes here.

android 29 introduces a dedicated Intent.identifier aimed at handling this case but we'll have to wait awhile until 29 becomes the minimum version

Can't we use this new API with if (SDK_INT >= 29)? Not sure what would be the advantage though, except having a fancier codebase :)

ouchadam · 2021-10-25T10:24:12Z

Can't we use this new API with if (SDK_INT >= 29)? Not sure what would be the advantage though, except having a fancier codebase :)

😄 I think that would be the only advantage, we would have to support an extra code branch in the notifications and deeplinks 😢

ouchadam mentioned this pull request Oct 22, 2021

Recurrence of #2605: Tapping drawer having more than one room in notifications gives "malformed link" error #4267

Closed

ouchadam commented Oct 22, 2021

View reviewed changes

bmarty reviewed Oct 22, 2021

View reviewed changes

filters the unique notification uris from the link handling

06b3cc3

- fixes malformed url errors appearing for uri we only create to force uniqueness in the notifications

ouchadam force-pushed the feature/adm/malformed-group-link branch from 8d0a22f to 06b3cc3 Compare October 22, 2021 17:29

ouchadam commented Oct 25, 2021

View reviewed changes

bmarty approved these changes Oct 25, 2021

View reviewed changes

ouchadam added 2 commits October 25, 2021 10:51

fixing strange ide extract

325e781

adding changelog entry

55c00a0

bmarty enabled auto-merge October 25, 2021 10:03

bmarty merged commit 8b6e018 into develop Oct 25, 2021

bmarty deleted the feature/adm/malformed-group-link branch October 25, 2021 10:20

Fixing grouped notification causing malformed url #4320

Fixing grouped notification causing malformed url #4320

Uh oh!

Conversation

ouchadam commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

ouchadam Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty commented Oct 25, 2021

Uh oh!

ouchadam commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ouchadam commented Oct 22, 2021 •

edited

Loading

github-actions bot commented Oct 22, 2021 •

edited

Loading

ouchadam Oct 22, 2021 •

edited

Loading

bmarty Oct 22, 2021 •

edited

Loading

ouchadam Oct 22, 2021 •

edited

Loading