[Location sharing] - Delete action on a live message (PSG-523) #6486

mnaturel · 2022-07-06T13:41:41Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adding the remove action to a live location share message. Aggregated summaries for a live are removed from database after it has been redacted.

Motivation and context

Closes #6437

Screenshots / GIFs

Tests

Go to a room
Share a live location
Long press on the message
Check the "Remove" action is available
Press the "Remove" action
Check a dialog to confirm is displayed
Confirm the remove
Check the message is replaced by a "Deleted message" texts
Check the location sharing service is stopped (in the case the live was not manually stopped before remove)

As there is a database migration, try to update the app from a previous version and check there is no crash at startup.

Tested devices

Physical
Emulator
OS version(s): Android 11

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

ouchadam · 2022-07-14T09:12:21Z

...in/java/org/matrix/android/sdk/internal/session/room/location/RedactLiveLocationShareTask.kt

+                roomId = params.roomId,
+                reason = params.reason
+        )
+        relatedEventIds.forEach { eventId ->


if one of these fails should we continue the redactions?

In here, we are only creating and posting tasks to redact the events. It means errors and retries will be handled internally inside the EventSenderProcessor. I don't know how to handle it differently since we want to have fast UI feedback when doing such action.

ouchadam · 2022-07-14T09:16:54Z

.../matrix/android/sdk/internal/session/room/location/DefaultRedactLiveLocationShareTaskTest.kt

+
+    @After
+    fun tearDown() {
+        unmockkAll()


~~if there's no statics being mocked we can avoid the tear down~~

just seen further down the FakeRealmConfiguration needs it!

ouchadam · 2022-07-14T09:25:16Z

...in/java/org/matrix/android/sdk/internal/session/room/location/RedactLiveLocationShareTask.kt

+    private fun redactEvent(eventId: String, roomId: String, reason: String?) {
+        Timber.d("redacting event of id $eventId")
+        val redactionEcho = localEchoEventFactory.createRedactEvent(roomId, eventId, reason)
+        localEchoEventFactory.createLocalEcho(redactionEcho)


this function name always catches me off guard as it's a create and persist unlike the other functions on localEchoEventFactory

I agree it can be confusing, I will try to find a better name for it.

I renamed it, see efa356e

ouchadam · 2022-07-14T09:36:26Z

.../matrix/android/sdk/internal/session/room/location/DefaultRedactLiveLocationShareTaskTest.kt

+    }
+
+    @Test
+    fun `given parameters when calling the task then it is correctly executed`() = runTest {


this test name feels a little bit generic 🤔 if we match the implementation then we would get when redacting then redacts event and related and creates redacted local echos

due to the amount of setup needed it makes the test a bit tricky to quickly read, what do you think about extracting some of the setup/verification?

val summary = createSummary(RELATED_EVENT_IDS) givenSummaryForEventId(AN_EVENT_ID, aggregatedSummaryEntity) val redactedEchos = givenCreatesRedactedLocalEchos(AN_EVENT_ID + RELATED_EVENT_IDS) defaultRedactLiveLocationShareTask.execute(params) verifyEventsRedacted(AN_EVENT_ID + RELATED_EVENT_IDS) verifyLocalEchosCreated(redactedEchos) verifyEventsRedacted(redactedEchos)

Agree thanks for the suggestions. I did some refactoring on this test. See 131f97b

ouchadam · 2022-07-14T09:39:25Z

...java/im/vector/app/features/home/room/detail/timeline/action/CheckIfCanRedactEventUseCase.kt

+import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent
+import javax.inject.Inject
+
+class CheckIfCanRedactEventUseCase @Inject constructor(


💯 for extracting the use case

ouchadam · 2022-07-14T09:45:35Z

...java/im/vector/app/features/home/room/detail/timeline/action/CheckIfCanRedactEventUseCase.kt

+
+    fun execute(event: TimelineEvent, actionPermissions: ActionPermissions): Boolean {
+        // Only some event types are supported for the moment
+        val canRedactEventTypes = listOf(EventType.MESSAGE, EventType.STICKER) +


using a single return when here could help reduce the amount of exit points (and in turn the complexity of the function)

Yes, in fact a simple if is easier to read here. See 1e872a8

ouchadam · 2022-07-14T09:54:18Z

.../main/java/im/vector/app/features/location/live/CheckIfLiveLocationShareIsRedactedUseCase.kt

+import timber.log.Timber
+import javax.inject.Inject
+
+class CheckIfLiveLocationShareIsRedactedUseCase @Inject constructor(


does this class need to be specific to LocationShare? looks like it might be able to become aIsEventRedactedUseCase

Yes you are right. I extracted the use case into a dedicated feature/redaction package so that it can be reused elsewhere. See 7252047

ouchadam · 2022-07-14T10:00:31Z

.../im/vector/app/features/home/room/detail/location/RedactLiveLocationShareEventUseCaseTest.kt

+    fun `given an event with empty id when calling use case then nothing is done`() = runTest {
+        val event = Event(eventId = "")
+
+        redactLiveLocationShareEventUseCase.execute(event = event, room = fakeRoom, reason = A_REASON)


we could also verify no interactions for reacting to help make the test more resilient

It was done implicitely by not mocking the call to the redact method in this case. But I added an explicit verification. See f784fa2

ouchadam

so many tests and usecases 😍 !

onurays · 2022-07-18T13:46:54Z

...java/im/vector/app/features/home/room/detail/location/RedactLiveLocationShareEventUseCase.kt

+
+    suspend fun execute(event: Event, room: Room, reason: String?) {
+        event.eventId
+                ?.takeUnless { it.isEmpty() }


onurays · 2022-07-18T13:50:27Z

...java/im/vector/app/features/home/room/detail/timeline/action/CheckIfCanRedactEventUseCase.kt

+
+        return event.root.getClearType() in canRedactEventTypes &&
+                // Message sent by the current user can always be redacted, else check permission for messages sent by other users
+                (event.root.senderId == activeSessionHolder.getActiveSession().myUserId || actionPermissions.canRedact)


Nice catch.

onurays · 2022-07-18T13:52:38Z

...in/java/im/vector/app/features/home/room/detail/timeline/helper/TimelineDisplayableEvents.kt

@@ -51,7 +51,7 @@ object TimelineDisplayableEvents {
            EventType.STATE_ROOM_JOIN_RULES,
            EventType.KEY_VERIFICATION_DONE,
            EventType.KEY_VERIFICATION_CANCEL,
-    ) + EventType.POLL_START + EventType.STATE_ROOM_BEACON_INFO
+    ) + EventType.POLL_START + EventType.STATE_ROOM_BEACON_INFO + EventType.BEACON_LOCATION_DATA


Why we added location data as displayable?

I added it because we want the redacted events to appear in the timeline (there are still hidden in the general case). I aligned the behavior on what is done in web. I think this is good to see the redacted events to inform the user the location events were redacted.

onurays

Very nice implementation. Thanks for the tests!

…on share

…vent id is empty

sonarqubecloud · 2022-07-19T14:22:21Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

66.5% Coverage
0.0% Duplication

mnaturel force-pushed the feature/mna/delete-lls branch from d7071ed to 332fcc5 Compare July 8, 2022 14:40

mnaturel changed the base branch from develop to feature/mna/collapse-deleted-events July 8, 2022 14:40

mnaturel force-pushed the feature/mna/delete-lls branch from a7bd31b to 97b6caf Compare July 11, 2022 12:54

mnaturel force-pushed the feature/mna/collapse-deleted-events branch from 8150790 to 67c7c0f Compare July 11, 2022 14:29

mnaturel force-pushed the feature/mna/delete-lls branch 3 times, most recently from d5d584a to 7ab6faf Compare July 12, 2022 15:07

mnaturel marked this pull request as ready for review July 12, 2022 15:09

mnaturel requested review from a team and ouchadam and removed request for a team July 12, 2022 15:09

ouchadam reviewed Jul 14, 2022

View reviewed changes

mnaturel force-pushed the feature/mna/collapse-deleted-events branch from 59632bf to add7a5f Compare July 18, 2022 08:05

mnaturel force-pushed the feature/mna/delete-lls branch from 7ab6faf to e845f33 Compare July 18, 2022 08:23

mnaturel requested a review from onurays July 18, 2022 13:25

onurays reviewed Jul 18, 2022

View reviewed changes

onurays approved these changes Jul 18, 2022

View reviewed changes

mnaturel force-pushed the feature/mna/delete-lls branch from 61e6f30 to 91559c2 Compare July 19, 2022 07:31

mnaturel force-pushed the feature/mna/collapse-deleted-events branch from 6eb05f8 to 9c61900 Compare July 19, 2022 12:05

Introducing use case to check if an event can be redacted

c095cc7

Maxime NATUREL added 18 commits July 19, 2022 14:05

Adding changelog entry

e26759f

Remove non necessary @mainthread annotations

bad4eba

Adding related event ids of live in entity

c404454

Adding task to redact live location share related events

c9273dd

Adding new method in location sharing service to redact a live locati…

237a5a1

…on share

Calling usecase to redact live location share event

391bf84

Prune event entity when redacted

d3ad8d8

Deleting summaries related to a redacted live location sharing

f6415b0

Redact beacon info event first to refresh timeline faster

63626b7

Stop sharing live location if live is redacted

8fb402a

Fix missing handling of redacted beacon location data events

1277376

DB migration

17227f1

Renaming a method to reflect its implementation

7b15193

Refactoring redact task unit tests

667b30f

Simplify logic of the use case to check if event can be redacted

99fc4b4

Moving use case to check if event is redacted into a dedicated package

2121ec5

Unit tests: adding verification that redaction does not happen when e…

7961525

…vent id is empty

Re-arranging imports

b2d7ef9

mnaturel force-pushed the feature/mna/delete-lls branch from 91559c2 to b2d7ef9 Compare July 19, 2022 12:07

Base automatically changed from feature/mna/collapse-deleted-events to develop July 19, 2022 14:39

mnaturel merged commit c3105c8 into develop Jul 19, 2022

mnaturel deleted the feature/mna/delete-lls branch July 19, 2022 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Location sharing] - Delete action on a live message (PSG-523) #6486

[Location sharing] - Delete action on a live message (PSG-523) #6486

mnaturel commented Jul 6, 2022 •

edited

Loading

ouchadam Jul 14, 2022

mnaturel Jul 18, 2022

ouchadam Jul 14, 2022 •

edited

Loading

ouchadam Jul 14, 2022

mnaturel Jul 18, 2022

mnaturel Jul 18, 2022

ouchadam Jul 14, 2022 •

edited

Loading

mnaturel Jul 18, 2022

ouchadam Jul 14, 2022

ouchadam Jul 14, 2022

mnaturel Jul 18, 2022

ouchadam Jul 14, 2022

mnaturel Jul 18, 2022

ouchadam Jul 14, 2022

mnaturel Jul 18, 2022

ouchadam left a comment

onurays Jul 18, 2022

onurays Jul 18, 2022

onurays Jul 18, 2022

mnaturel Jul 18, 2022

onurays left a comment

sonarqubecloud bot commented Jul 19, 2022

[Location sharing] - Delete action on a live message (PSG-523) #6486

[Location sharing] - Delete action on a live message (PSG-523) #6486

Conversation

mnaturel commented Jul 6, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurays left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 19, 2022

mnaturel commented Jul 6, 2022 •

edited

Loading

ouchadam Jul 14, 2022 •

edited

Loading

ouchadam Jul 14, 2022 •

edited

Loading