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

Added modal for creating/updating datasets #310

Merged
merged 5 commits into from
Oct 10, 2023
Merged

Conversation

zonia3000
Copy link
Collaborator

@zonia3000 zonia3000 commented Oct 10, 2023

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

Closes #299

@zonia3000
Copy link
Collaborator Author

@tcompa Let me know if the modals work as expected, then we should define better how the "open" feature should be implemented. At the moment the page shown with the open button has not been modified.

@zonia3000 zonia3000 marked this pull request as draft October 10, 2023 07:10
@tcompa
Copy link
Collaborator

tcompa commented Oct 10, 2023

Brief recap of the points discussed live:

  1. "open" page should not allow any edit
  2. dataset.type should not be required
  3. Setting "custom" dataset.type is currently not working
  4. Let's add a piece of logic to parse and re-format the most common instance of 422 error due to pydantic validation of the request body. This is useful here, but then it would be nice to have it everywhere - to make error messages more readable. See example here: Remove fractal-common and remove/review API-call validation fractal-client#548 (comment)
  5. Let's review the user flow for when a dataset creation partially fails (dataset is created, but one of the resources' paths is wrong) - as discussed.

@zonia3000 zonia3000 marked this pull request as ready for review October 10, 2023 12:02
@tcompa
Copy link
Collaborator

tcompa commented Oct 10, 2023

"open" page should not allow any edit

Now working as expected

dataset.type should not be required

Now working as expected

Setting "custom" dataset.type is currently not working

Now working as expected

Let's add a piece of logic to parse and re-format the most common instance of 422 error due to pydantic validation of the request body. This is useful here, but then it would be nice to have it everywhere - to make error messages more readable. See example here: fractal-analytics-platform/fractal-client#548 (comment)

Now working as expected, see red message in the bottom:
image

Let's review the user flow for when a dataset creation partially fails (dataset is created, but one of the resources' paths is wrong) - as discussed.

Now working as expected, see screenshot above.

Copy link
Collaborator

@tcompa tcompa left a comment

Choose a reason for hiding this comment

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

I played around with latest version, and the main feature is as expected. If there remain glitches or weird edge cases, we can address them elsewhere.

I'd say this is good for merging, unless @zonia3000 has something more to add/review.

@jluethi
Copy link
Collaborator

jluethi commented Oct 10, 2023

Happy to test later today. But that can be after the merge, I'd open new issues if they arise :)

@tcompa
Copy link
Collaborator

tcompa commented Oct 10, 2023

Happy to test later today. But that can be after the merge, I'd open new issues if they arise :)

I'd say you can wait and directly test the upcoming v0.6 (ref #308), to avoid having too many back-and-forth rounds.

@zonia3000
Copy link
Collaborator Author

I'd say this is good for merging, unless @zonia3000 has something more to add/review.

For me it can be merged.

@tcompa tcompa merged commit f61d59f into main Oct 10, 2023
@tcompa tcompa deleted the dataset-crud-updated branch October 10, 2023 13:13
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.

Revamp dataset create/update feature
3 participants