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

Fix dragging annotation into a different orga’s dataset #7816

Merged
merged 5 commits into from
May 27, 2024

Conversation

fm3
Copy link
Member

@fm3 fm3 commented May 22, 2024

When drag’n’dropping an annotation into a dataset in view-mode, the annotation is uploaded into that dataset, regardless of what dataset is reference din the NML tags.

This PR now makes this available if the organization name has changed as well.

URL of deployed dev instance (used for testing):

Steps to test:

  • Create annotation, download
  • In the NML file, edit the organization name in the experiment tag to some non-existing orga, or remove it completely
  • Uploading that annotation by dragging it into a dataset view mode should still work, and create an annotation for that dataset.

Issues:


@fm3 fm3 marked this pull request as ready for review May 22, 2024 09:53
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for this improvement 🙏

I only found one potential improvement of the changes. But as I am unsure whether my suggestion keeps the same functionality, feel free to ignore it and merge

Comment on lines +66 to 67
organizationName = if (overwritingDatasetName.isDefined) overwritingOrganizationName
else parseOrganizationName(parameters \ "experiment")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the if-check check for overwritingOrganizationName being defined and not for overwritingDatasetName? Or is there some other place where it is ensured that in case overwritingDatasetName is defined, then overwritingOrganizationName is also defined?

And maybe the same syntax as in the line above can be used here as well, as imo the code is more readable this way. Or is there a semantic difference between my suggestion and your code?

Suggested change
organizationName = if (overwritingDatasetName.isDefined) overwritingOrganizationName
else parseOrganizationName(parameters \ "experiment")
organizationName = overwritingOrganizationName.getOrElse(parseOrganizationName(parameters \ "experiment"))

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there is in fact a difference. I want the overwritingOrganizationName, even if it is None, if the overwritingDatasetName is defined.

This case will probably not happen, because the frontend sends both or neither. But from the backend point of view, the organizationName from the NML becomes invalid once the datasetName is overwritten (this was already the case before this PR). So in that moment I want to yield the None, so that it will later be filled by the disambiguation logic via dataset name, and not the organizationName from the NML.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Please wait before merging: I am running into CSP errors in Chrome (version 125.0.6422.60) & Ubuntu:
image

Let's tackle this in this PR as well, as the zip uploading in an annotation does not work on the master as well (with the chrome version)

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Found a CSP fix 🎉

@fm3 fm3 merged commit c6eade1 into master May 27, 2024
2 checks passed
@fm3 fm3 deleted the upload-annotation-orga-specific branch May 27, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading Annotation into dataset should also include overwritingOrganizationName
2 participants