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

Properly reserve dataset when putting datasource (addRemote) #7221

Merged
merged 5 commits into from
Jul 24, 2023

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Jul 20, 2023

Datastore now calls reserveDataSourceUpload when adding a datasource. This sets both uploader and folderId properly, as well as performs the correct write access checks.

(Note that this is a subset of #7176 as I expect that to be blocked for a while, but we should fix this bug. I copied the changes to the two backend source files from that PR)

Steps to test:

  • Add remote dataset as non-admin user, see that this works for folders you have write access for (and not for others)
  • Check that the folder is set correctly. (Also, uploader should be set in db)

TODOs:

  • Frontend: pass folderId to the put route instead of calling updatePartial afterwards.

Issues:


@philippotto philippotto removed their assignment Jul 24, 2023
@fm3 fm3 marked this pull request as ready for review July 24, 2023 09:49
@fm3
Copy link
Member Author

fm3 commented Jul 24, 2023

@philippotto thanks! This works well when hitting the add button while navigated to a folder. However, in the upload view, there is a dropdown for specifying the folder. This is missing in the add remote view. Do you think this can be unified? I think having the menu (or at least a note saying where the dataset will go) would be good for the usability.

@fm3 fm3 requested a review from frcroth July 24, 2023 09:59
@philippotto
Copy link
Member

@philippotto thanks! This works well when hitting the add button while navigated to a folder. However, in the upload view, there is a dropdown for specifying the folder. This is missing in the add remote view. Do you think this can be unified? I think having the menu (or at least a note saying where the dataset will go) would be good for the usability.

Maybe I'm missing something, but the add-remote-view has the dropdown you want. However, it only appears after one has explored the first URI. I can see why showing it earlier might make sense, but due to the current explore-configure flow, the UI appears later. Changing that would take some effort which isn't worth it in my opinion.

image

@fm3
Copy link
Member Author

fm3 commented Jul 24, 2023

I see, thanks! I missed it 🙈 Yes, let’s keep it like that for now, then :)

Copy link
Member

@frcroth frcroth left a comment

Choose a reason for hiding this comment

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

Very efficient! Closing 3 issues with one PR

@fm3 fm3 merged commit 734b919 into master Jul 24, 2023
2 checks passed
@fm3 fm3 deleted the add-remote-reserve branch July 24, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants