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

User select and sharing of sample page with creators #830

Merged
merged 5 commits into from
Nov 29, 2024
Merged

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Aug 2, 2024

This PR adds a user select and toggleable editable option to the creator list for items.

It should prevent the user from removing themselves, and should potentially prevent the user from removing other people (i.e. append-only share), or some more tortured logic where the "first" creator in the list takes precedence.

This also adds a new API route PATCH /items/<refcode>/permissions for this use case, with some tests.

Works similarly to the toggleable collection block, except that API call is made when the editing state is completed, and has a confirmation dialog.

Some outstanding things (for this PR or otherwise):

  • Error handling when the API update fails, e.g., if the creator tries to remove themselves. For me, the the error seems to get caught by the dev server before it gets back to the component to reset the value.
  • Cypress testing... tricky since we're not ever logged in during the current e2e tests

@ml-evs ml-evs added the webapp For issues/PRs pertaining to the web interface label Aug 2, 2024
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 79.48718% with 8 lines in your changes missing coverage. Please review.

Project coverage is 68.62%. Comparing base (f58a5ed) to head (f7aa49b).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
pydatalab/src/pydatalab/routes/v0_1/items.py 79.48% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #830      +/-   ##
==========================================
+ Coverage   68.49%   68.62%   +0.12%     
==========================================
  Files          62       62              
  Lines        3955     3993      +38     
==========================================
+ Hits         2709     2740      +31     
- Misses       1246     1253       +7     
Files with missing lines Coverage Δ
pydatalab/src/pydatalab/routes/v0_1/items.py 82.52% <79.48%> (-0.14%) ⬇️

Copy link

cypress bot commented Aug 2, 2024

datalab    Run #2843

Run Properties:  status check passed Passed #2843  •  git commit 902e854bc2 ℹ️: Merge f7aa49bf9904e929c7560f9624d3a48f060c272c into f58a5eddb99e734efbb5585b7466...
Project datalab
Branch Review ml-evs/creators-ui
Run status status check passed Passed #2843
Run duration 07m 31s
Commit git commit 902e854bc2 ℹ️: Merge f7aa49bf9904e929c7560f9624d3a48f060c272c into f58a5eddb99e734efbb5585b7466...
Committer Matthew Evans
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 471
View all changes introduced in this branch ↗︎

@ml-evs ml-evs force-pushed the ml-evs/creators-ui branch 2 times, most recently from 493b915 to 9aa4708 Compare August 4, 2024 16:53
@ml-evs ml-evs changed the title WIP user select and sharing of sample page by creators User select and sharing of sample page with creators Aug 4, 2024
@ml-evs ml-evs mentioned this pull request Aug 4, 2024
@ml-evs ml-evs force-pushed the ml-evs/creators-ui branch 2 times, most recently from c0bb29a to a055f7a Compare October 3, 2024 12:23
@ml-evs ml-evs added this to the v0.5.x milestone Oct 8, 2024
@ml-evs ml-evs added the enhancement New feature or request label Nov 6, 2024
@ml-evs ml-evs marked this pull request as ready for review November 6, 2024 22:57
@ml-evs ml-evs force-pushed the ml-evs/creators-ui branch 4 times, most recently from 2ea51fb to e9cae07 Compare November 11, 2024 23:21
@BenjaminCharmes BenjaminCharmes linked an issue Nov 20, 2024 that may be closed by this pull request
@ml-evs ml-evs force-pushed the ml-evs/creators-ui branch 2 times, most recently from f0c37f6 to b0a65ff Compare November 28, 2024 17:59
ml-evs and others added 4 commits November 28, 2024 23:14
- Run Pydantic validation in search-users endpoint

- Ensure creator ID is returned by `/items` endpoint within creators object

Add specialised route for adding creators to items

- Make it so users cannot remove themselves nor the original creator from creators lists
Add contact_email to the API response when searching user to display Gravatar Icon inside the Creators vSelect

Filter user that are already been selected to not appear in the vSelect

Update creators doesn't trigger the 'Unsaved change'

Add toggleable creators to cell type, and try to limit creator display names
@ml-evs ml-evs force-pushed the ml-evs/creators-ui branch 2 times, most recently from 7b3dabd to 047fe7d Compare November 29, 2024 00:13
- Make sure creators order is preserved in lookup

- Better handle case of item with no creators

- Better error messages and handling of edge cases

- Better client-side handling of edge cases and invalid inputs in creator selection

- Tweak permissions test

- Fix typo...

- Use data-test-id for better encapsulation of dropdown testing
@ml-evs
Copy link
Member Author

ml-evs commented Nov 29, 2024

This has been deployed on @PeterKraus's deployment for a few weeks now and seems to be working well (pending future enhancements to also apply it to collections). Since all I've done since then is add data-testid's to get the e2e tests working, I'm going to take responsibility and merge this now...

@ml-evs ml-evs merged commit ab60779 into main Nov 29, 2024
17 checks passed
@ml-evs ml-evs deleted the ml-evs/creators-ui branch November 29, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request webapp For issues/PRs pertaining to the web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to pass component props within DynamicDataTable
3 participants