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

Url based remote dataset import #7844

Merged
merged 37 commits into from
Jul 10, 2024
Merged

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented May 29, 2024

URL of deployed dev instance (used for testing):

Steps to test:

  • pick any remote dataset, and use route /import?url=<...>. Ideally the dataset should open after a bit of loading
  • make sure the usual remote DS upload is still working
  • try causing an error during upload (no/wrong URL, use dataset twice etc) and check that you are seeing the upload form wirh an appropriate error
  • if you open a dataset that has been imported before, you should directly be redirected to the dataset view

TODOs:

  • fix modal The explored data has a different voxel size from the datasource that was already loaded.
  • add animation in order not to show form in happy path
  • infer name and automatically submit second form in happy path
  • right now its hard to load the real errors in handleStoreDataset (form.validateFields()). maybe theres a good way to print only the relevant fields?
  • route to login if needed
    • route to form afterwards: only keep URI parameter
  • code self-review and clean-up.
    • prevent mutliple isNameValid calls! handleStoreDataset seems to only be called once...
  • TEST!!!
  • fix theme of PW field in add layer modal
  • append url hash to name
  • if url has been imported before, open existing DS
  • fix that brain spinner is jumping when loading DS (edit: skipped for now. I think this happens if the navbar changes from text to icons, I didnt find an easy reason and it doesnt seem worth to spend more time there
  • change label if url is edited

Issues:


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

@dieknolle3333 dieknolle3333 self-assigned this May 29, 2024
} catch (_e) {
Toast.error(
"The current datasource config contains invalid JSON. Cannot add the new Zarr/N5 data.",
);
return;
}
if (existingDatasource != null && !_.isEqual(existingDatasource.scale, newDataSource.scale)) {
if (
existingDatasource?.scale != null &&
Copy link
Contributor Author

@dieknolle3333 dieknolle3333 Jun 17, 2024

Choose a reason for hiding this comment

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

this was failing in my case because the dataSourceJson didnt contain the scale at some point. It seems to me that its still making sense, but I just want to make sure that its not problematic in another case.

  • obvs check whether usual remote DS upload is still working fine

@dieknolle3333 dieknolle3333 marked this pull request as ready for review July 1, 2024 15:09
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

looking good overall! left a couple of comments :)

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

great! I didn't do a final test, since I assume you will do (or have done) this before the merge :)

@dieknolle3333
Copy link
Contributor Author

Amazing, thank you! Yes, I tested it just now and it still seems to be working good. I will quickly test it again tomorrow right before merging.
Sorry if I am being nit-picky, but do you think the other thread in this commit is resolved? Just trying to make sure I didnt miss anything.

@dieknolle3333 dieknolle3333 merged commit a58a78e into master Jul 10, 2024
2 checks passed
@dieknolle3333 dieknolle3333 deleted the url-based-remote-dataset-import branch July 10, 2024 08:46
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.

URL-based remote dataset import
2 participants