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

Filter datasets by organization, use that in task creation view #6377

Merged
merged 6 commits into from
Aug 10, 2022

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Aug 5, 2022

  • Public datasets of other organizations are no longer listed during task creation. (Previously they were listed but still ultimately failed, compare Allow task creation for public datasets of other organizations #3568)
  • Added new filter options for the dataset list api at api/datasets: organizationName: String, onlyMyOrganization: Boolean, uploaderId: String

Steps to test:

  • Set up two organizations, add datasets for both, set some datasets to public
  • In task creation view you should not see other organization’s datasets in the dropdown menu, even if they are public
  • try new filter options for dataset list: organizationName: String, onlyMyOrganization: Boolean, uploaderId: String

Issues:


@fm3 fm3 self-assigned this Aug 5, 2022
@fm3 fm3 marked this pull request as ready for review August 5, 2022 10:47
@fm3 fm3 requested review from philippotto and jstriebel August 5, 2022 10:47
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.

front-end lgtm 👍

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

The backend code looks mostly fine, I'd just like to have a Swagger annotation for the parameters of the datasetList endpoint ✍️

@@ -10,6 +10,10 @@ object DefaultConverters {
}
}

implicit object StringToString extends Converter[String, String] {
Copy link
Contributor

Choose a reason for hiding this comment

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


(Why is this needed? Doesn't do much harm, just wondering…)

Copy link
Member Author

Choose a reason for hiding this comment

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

The current implementation of Filters extracts the get parameters, and uses these Converters to type them. There wasn’t a string one before, and this implicit method was needed.

However, I guess we should stop using this method, and go for explicit get parameters, also allowing for standard swagger anotations. It may be that I can remove these converters then. I’ll have a look

@fm3
Copy link
Member Author

fm3 commented Aug 9, 2022

@jstriebel I added swagger annotations and listed the parameters explicitly, including types. Please have another look!

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

Perfect, LGTM 👍

@fm3 fm3 merged commit c0e1c4b into master Aug 10, 2022
@fm3 fm3 deleted the task-only-orga-datasets branch August 10, 2022 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants