Add collaboration UI #1491

danilo04 · 2021-09-13T16:04:41Z

Fix

This PR adds a UI for adding/removing collaborators to a note. Sorry for the size of the PR but I couldn't break more the changes to bring collaboration to Android (they already broken in 4 different issues/PRs #1473).

Note: I have stressed tested the UI by adding and removing collaborators as fast as I could and there are some cases in which collaborators are deleted from the list but then added. I presume that this is mostly due to a race condition on the Simperium library. I will track the issue in Simperium/simperium-android#238.

Test

Go to a note
Tap on the menu on the top right corner (dot ...)
Tap on collaborators
Tap on Add Collaborator and type anything that is not a valid email. A message invalid collaborator should be shown.
Write a valid email address and tap on Accept. Should update the layout to list of collaborators
Tap on the cross icon to remove the collaborator. Should update the layout to show the initial message
Tap on Add Collaborator and enter an email address from another valid simplenote account
The note should be shared to the other account

Review

Only one developer and one designer are required to review these changes, but anyone can perform the review.

Release

The SimperiumCollaboratorsRepository now has a dependency for Bucket<Note>, thus, all tests that use the instance directly had to be updated.

Given that notes are synchronized across different devices, there may be the case in which a user will try to get the collaborators of a note that was deleted or is in the trash. For those cases, we cannot add collaborators.

Created the getNote method that will be reused in addCollaborator and getCollaborators since these methods need a Note object.

Collaborator validation should not be responsibility of addCollaborators.

Since loading a note does a database query, we change all the methods in SimperiumCollaboratorsRepository to suspend methods. To be able to test properly the repository, we need to inject the dispatcher, so we updated the dispatcher in all the test and created a provider for Hilt.

Tests on model does not require emulator. They can run fast and are better suited to run as unit tests instead of connected tests.

Make explicit that TagValidationResult is part and just used in ValidateTagUseCase.

To be able to test this method, we need to mock the whole class. We mock CollaboratorsRepository and all the calls that the tests made.

danilo04 · 2021-09-21T04:03:48Z

Thank you for the thorough review as always @ParaskP7. I have made most of the changes requested and commented in some requests that I did not apply any change. Let me know WDYT?

We have commented over other channels about the long PR and the duplicated commits.

ParaskP7 · 2021-09-21T09:32:21Z

👋 @danilo04 !

Thank you for the thorough review as always @ParaskP7. I have made most of the changes requested and commented in some requests that I did not apply any change. Let me know WDYT?

Thank YOU for applying the suggestions in such a graceful way. I have just left a couple of comments unresolved for us to have one final discussion. However, I don't want to block this PR anymore and will approve it no matter. Feel free to merge it when you feel you are ready.

We have commented over other channels about the long PR and the duplicated commits.

💯

This is mostly for testing purposes since it is easier to inject a test dispatcher to avoid multithreading bugs in the testing library for coroutines.

SylvesterWilmott · 2021-09-21T16:07:33Z

Haven't had an opportunity to fully test but some observations so far:

"Add Collaborator" button should not be a flat button but should instead be a tappable row.
The list of collaborators has additional padding on the right, eg. the touch feedback of the "x" is cut off on the right-hand side. This is in contrast to similar layouts like tags edit view.
Once a collaborator is added, the email appears in the tags list.
The "Email Address" input blue colour does not adapt to dark mode. Blue30 should be used here.
The "x" icons in the collaborator list don't adapt to dark mode. Gray30 should be used here.

Even though the dialog was using the correct mode (light or dark), the view inside the dialog was not. To make the view inside the dialog mode aware, we use the context theme wrapper to inflate the layout.

danilo04 · 2021-09-22T04:49:37Z

Hi @SylvesterWilmott 👋 . Thanks for the review. I have made the following changes:

"Add Collaborator" button should not be a flat button but should instead be a tappable row.

Do you mean the whole row where Add Collaborator is, is tappable?
I have made the whole row tappable in 74171ed but I am not sure if that is what you mean.

The list of collaborators has additional padding on the right, eg. the touch feedback of the "x" is cut off on the right-hand side. This is in contrast to similar layouts like tags edit view.

I have removed the padding but part of the touch feedback is cut off similar to the delete button in edit tags (fixed in 74171ed).

Once a collaborator is added, the email appears in the tags list.

~~I am not sure about this. Could you expand a little bit?~~
This will be handled in a different PR.

The "Email Address" input blue colour does not adapt to dark mode. Blue30 should be used here.

Fixed in 84ee1e4.

The "x" icons in the collaborator list don't adapt to dark mode. Gray30 should be used here.

Fixed in 74730be.

SylvesterWilmott · 2021-09-23T09:03:38Z

Thanks for the changes @danilo04

All looks good. Just one more request, let's add the touch feedback to the "Add Collaborator" row.

danilo04 · 2021-09-23T15:31:20Z

Hi @SylvesterWilmott.

Just one more request, let's add the touch feedback to the "Add Collaborator" row.

Fixed in 208f343. Let me know if it works OK on your part.

SylvesterWilmott · 2021-09-24T08:34:30Z

It looks like this PR was merged before the needs design review label was removed but there are a couple more issues we can tackle in the next PR.

The "Add Collaborator" row should be 48dp tall with 8dp top and bottom margins. This means the area that is tappable should only be 48dp. This is illustrated in Zeplin by clicking on the bounding boxes in the design:

It looks like it's possible to add yourself as a collaborator. While we validate the email, let's also make sure that the email doesn't match the email of the currently logged in account. In this event we can show the error You cannot add yourself as a collaborator in the red error text below the input.

danilo04 added 30 commits September 9, 2021 10:45

Added tests for getCollaborators method from CollaboratorsRepository

f190dd7

The SimperiumCollaboratorsRepository now has a dependency for Bucket<Note>, thus, all tests that use the instance directly had to be updated.

Format file for git

8504289

Fix tests for CollaboratorsRepository

e37d61d

Add implementation for getCollaborators in CollaboratorsRepository

e3b5a75

Refactor CollaboratorsRepositoryTest to delete duplicate code

8d0c126

Add tests for addCollaborator in CollaboratorsRepository

650f353

Refactor getCollaborators to use a generic method that is reused

4323206

Created the getNote method that will be reused in addCollaborator and getCollaborators since these methods need a Note object.

Add implementation of addCollaborator for CollaboratorsRepository

6077431

Remove validation in CollaboratorsRepository

78804ca

Collaborator validation should not be responsibility of addCollaborators.

Add tests for removeCollaborator

cb02c32

Implement removeCollaborator

f23d6b3

Fix format in GetTagsUseCase

f56fee0

Move NoteTest to unit tests directory since it doesn't require emulator

4d42dd5

Tests on model does not require emulator. They can run fast and are better suited to run as unit tests instead of connected tests.

Add tests for addTag method from Note class

cf7b357

Add tests for removeTag method from Note class

b1c96ca

Reformat comments in GetTagsUseCase

f92cca8

Refactor TagValidationResult to be part of ValidateTagUseCase

a59dacf

Make explicit that TagValidationResult is part and just used in ValidateTagUseCase.

Refactor isTagValid to a more idiomatic Kotlin way

cb75708

Reformat stub call in test case

542abb0

Refactor allTagsShouldFilterCollaborators test to remove duplication

9ff6d51

Refactor to use lambda instead of anonymous classes NoteEditorFragment

f480b6c

Refactor to make NoteEditorEvent part of NoteEditorViewModel

ace9aee

Rename CollaboratorsRepositoryTest to SimperiumCollaborators...

3c1893c

Remove unnecessary empty line

6526a9a

Update tests cases after rebase of branch to develop

a1226cd

Reformat test cases

ec634b3

Reformat test cases

9a565bc

Split tests that checked two different properties.

cc75ba6

danilo04 added 11 commits September 20, 2021 14:34

Remove blank spaces

833acdf

Refactor toolbar setup logic into its own method

198ee8b

Refactor setObservers for consistency

f731ffc

Refactor calling submitList on CollaboratorsAdapter

78df3d6

Remove incorrect comment

d321b84

Refactor callback format to remove unused variables

a2229c9

Reformat activity_collaborators.xml

af1df21

Refactor views in CollaboratorsActivity to start visibility gone

0918454

Reformat tests to follow arrange/act/assert structure

8cd381c

Reformat tests to follow arrange/act/assert structure in repositories

9c93954

Add tests for calls to collaboratorsChanged() method

56de650

To be able to test this method, we need to mock the whole class. We mock CollaboratorsRepository and all the calls that the tests made.

danilo04 requested a review from ParaskP7 September 21, 2021 04:01

ParaskP7 approved these changes Sep 21, 2021

View reviewed changes

Add IO dispatcher parameter to SimperiumTagsRepository

32eca5b

This is mostly for testing purposes since it is easier to inject a test dispatcher to avoid multithreading bugs in the testing library for coroutines.

danilo04 added 5 commits September 21, 2021 21:55

Change padding in collaborators row

74171ed

Fix cross color for dark mode in collaborators table rows

74730be

Inflate layout for dialog using the context themed wrapper

84ee1e4

Even though the dialog was using the correct mode (light or dark), the view inside the dialog was not. To make the view inside the dialog mode aware, we use the context theme wrapper to inflate the layout.

Request focus on text input after the dialog logs

4fc4d25

Made inflate layout theme aware for edit tag dialog

48d67ef

Add touch feedback to Add Collaborator row button

208f343

Align message text with add collaborator button

dbaf20c

danilo04 merged commit 15796db into develop Sep 23, 2021

danilo04 deleted the issue/1479-add-collaboration-ui branch September 23, 2021 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add collaboration UI #1491

Add collaboration UI #1491

danilo04 commented Sep 13, 2021 •

edited

Loading

danilo04 commented Sep 21, 2021

ParaskP7 commented Sep 21, 2021

SylvesterWilmott commented Sep 21, 2021

danilo04 commented Sep 22, 2021 •

edited

Loading

SylvesterWilmott commented Sep 23, 2021

danilo04 commented Sep 23, 2021

SylvesterWilmott commented Sep 24, 2021 •

edited

Loading

Add collaboration UI #1491

Add collaboration UI #1491

Conversation

danilo04 commented Sep 13, 2021 • edited Loading

Fix

Test

Review

Release

danilo04 commented Sep 21, 2021

ParaskP7 commented Sep 21, 2021

SylvesterWilmott commented Sep 21, 2021

danilo04 commented Sep 22, 2021 • edited Loading

SylvesterWilmott commented Sep 23, 2021

danilo04 commented Sep 23, 2021

SylvesterWilmott commented Sep 24, 2021 • edited Loading

danilo04 commented Sep 13, 2021 •

edited

Loading

danilo04 commented Sep 22, 2021 •

edited

Loading

SylvesterWilmott commented Sep 24, 2021 •

edited

Loading