Skip to content

Merge "Shared Annotations" with "My annotations" #6230

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

Merged
merged 37 commits into from
Jun 20, 2022
Merged

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented May 20, 2022

  • @fm3 Could you have a look at my backend changes? I'm not sure whether this is the right approach (the "compact writes" got a bit more expensive due to the additional look ups). Also, I'm not sure how the pagination should be built when merging "own" with "shared" annotations.

URL of deployed dev instance (used for testing):

Steps to test:

  • Set up two users (one should already exist; for the second one, register in an incognito browser tab and enable the account from the first user)
  • Create multiple annotations from both accounts
  • Assign a common team to both users and share some annotations with that team
  • The dashboard "annotation" tab should always show the own annotations and the ones which are shared with that user
  • Shared annotations should not be editable via the "Archive", "Archive All" or "Edit Name" buttons.
  • Test the filtering UI in the "Owners & Teams" column

Todo:

  • unify user and owner attribute in annotations

Issues:


(Please delete unneeded items, merge only when none are left open)

@philippotto philippotto requested a review from fm3 May 20, 2022 13:02
@philippotto philippotto changed the title [WIP] Merge "Shared Annotations" with "My annotations" Merge "Shared Annotations" with "My annotations" May 30, 2022
@philippotto philippotto requested a review from daniel-wer June 2, 2022 12:55
@philippotto
Copy link
Member Author

@daniel-wer I think this PR is ready for review :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Works really well! Thanks for drive-by-improving the types!

Some smaller issues I noticed during testing:

  • If I select the annotations tab, the URL switches to https://dashboardcollab.webknossos.xyz/dashboard/shared - I would have expected /annotations, but maybe this is by design
  • The Owner/Teams filter in the table header works great, but doesn't seem to be taken into consideration by the "Archive All" button. Although only one annotation (which is read-only and cannot be archived) is visible due to the filter, the message reads "Are you sure you want to archive all 2 explorative annotations matching the ..."
  • If I archive or reopen an annotation and then switch to the respective view where it can be found, it turns up twice. A page reload "repairs" that.

@philippotto
Copy link
Member Author

Thanks for catching these bugs! I fixed them all now (plus some additional potential culprits) and also opened #6275, since the entire tab could use some rethinking. Please have another look :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

There is one issue left with the "Archive All" functionality - apart from that LGTM 👍

@philippotto
Copy link
Member Author

Great catch, @daniel-wer 👍 It took me a bit to find a good workaround, since I found the onChange prop to be unsuitable to fix all issues. I got a solution which works with the currently visible items of the active page. It's not the same as before, since all other pages are ignored, but I think this even has benefits (the user will only mutate the items which are really visible). Please have a look at my latest commit (and the comments it contains) :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice that you found a workaround! I agree that only modifying the visible results from the active page is even better than the old behavior 👍

@philippotto philippotto merged commit ef44b6c into master Jun 20, 2022
@philippotto philippotto deleted the dashboard-collab branch June 20, 2022 10:54
philippotto added a commit that referenced this pull request Jun 20, 2022
philippotto added a commit that referenced this pull request Jun 20, 2022
philippotto added a commit that referenced this pull request Jun 20, 2022
hotzenklotz added a commit that referenced this pull request Jun 23, 2022
…type

* 'master' of github.com:scalableminds/webknossos:
  fix error when loading agglomerate skeleton for single-segment agglomerate (#6294)
  Editable Mappings aka Supervoxel Proofreading (#6195)
  Increase maximum interpolation depth to 100 (#6292)
  Add download modal to dataset view actions (#6283)
  Drop "Explorational" from info tab (#6290)
  Allow version history view in annotations not owned by you (#6274)
  Bucket loading meter (#6269)
  Revert "Merge "Shared Annotations" with "My annotations" (#6230)" (#6286)
  Merge "Shared Annotations" with "My annotations" (#6230)
fm3 added a commit that referenced this pull request Jul 7, 2022
* Revert "Revert "Merge "Shared Annotations" with "My annotations" (#6230)" (#6286)"

This reverts commit de22b53.

* broaden access context and add error chains to annotation public writes

* llist access query

* remove time logging

* naming things

Co-authored-by: Florian M <[email protected]>
Co-authored-by: Florian M <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework annotations table towards new collaboration features
3 participants