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.

Sign up for GitHub

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

Jump to bottom

feat: enable refback column data to be included in csv/excel downloads (will be ignored during import) #4705

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

feat: enable refback column data to be included in csv/excel downloads (will be ignored during import) #4705

wants to merge 20 commits into from
+216 −194

Conversation

mswertz
Copy link
Member

@mswertz mswertz commented Feb 11, 2025 •
edited
Loading

closes #4223

What are the main changes you did

  • removed filter of refback

How to test

  • download and see refbacks are included

Checklist

  • is this really what we want? it is not the same as what we would import although we would not fail on it.

@mswertz mswertz requested a review from hslh February 12, 2025 06:13
@hslh
Copy link
Contributor

hslh commented Feb 12, 2025

Checklist

* [ ]  is this really what we want? it is not the same as what we would import although we would not fail on it.

I think showing a warning/notice on import saying refback columns are ignored should be sufficient. If it's still a problem, adding an option/checkbox/something saying 'include refback columns' on the download screen might be a solution.

hslh
hslh reviewed Feb 12, 2025
View reviewed changes
Copy link
Contributor

@hslh hslh left a comment

Choose a reason for hiding this comment

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

Looks good, download/upload for directory-demo works. For catalogue-demo it also works, except that I get a zip-file of 0 bytes when downloading everything as csv.zip: https://preview-emx2-pr-4705.dev.molgenis.org/catalogue-demo/api/zip

@mswertz
Copy link
Member Author

mswertz commented Feb 12, 2025

Looks good, download/upload for directory-demo works. For catalogue-demo it also works, except that I get a zip-file of 0 bytes when downloading everything as csv.zip: https://preview-emx2-pr-4705.dev.molgenis.org/catalogue-demo/api/zip

that is new? then we found a bug we need to first fix

@mswertz
Copy link
Member Author

mswertz commented Feb 12, 2025

Looks good, download/upload for directory-demo works. For catalogue-demo it also works, except that I get a zip-file of 0 bytes when downloading everything as csv.zip: https://preview-emx2-pr-4705.dev.molgenis.org/catalogue-demo/api/zip

that is new? then we found a bug we need to first fix

Ah, this is a missing feature having to do with selection of refback columns in combination with composite keys. Not so simple ;-)

@mswertz mswertz marked this pull request as draft February 12, 2025 21:12
@mswertz mswertz marked this pull request as ready for review February 18, 2025 23:03
@mswertz mswertz requested a review from harmbrugge February 19, 2025 06:18
harmbrugge
harmbrugge requested changes Feb 19, 2025
View reviewed changes
backend/molgenis-emx2-sql/src/main/java/org/molgenis/emx2/sql/SqlQuery.java Outdated Show resolved Hide resolved
backend/molgenis-emx2-sql/src/main/java/org/molgenis/emx2/sql/SqlQuery.java Show resolved Hide resolved
backend/molgenis-emx2-webapi/pet store_Pet1739385622139.zip Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Where is this used? For tests? then it should be moved to the resource folder

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, this shouldn't be in the PR, thanks

@hslh hslh self-requested a review February 19, 2025 12:21
hslh
hslh reviewed Feb 19, 2025
View reviewed changes
Copy link
Contributor

@hslh hslh left a comment

Choose a reason for hiding this comment

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

The issue w.r.t. downloading the csv.zip for catalogue-demo persists: https://preview-emx2-pr-4705.dev.molgenis.org/catalogue-demo/api/zip gets a 0-byte invalid file.

Getting all data as Excel gives an error https://preview-emx2-pr-4705.dev.molgenis.org/catalogue-demo/api/zip -> "Query failed: : column a0.internal identifiers.resource does not exist. "

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@mswertz
Copy link
Member Author

mswertz commented Feb 19, 2025

The issue w.r.t. downloading the csv.zip for catalogue-demo persists: https://preview-emx2-pr-4705.dev.molgenis.org/catalogue-demo/api/zip gets a 0-byte invalid file.

ah, though I fixed this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hslh hslh hslh left review comments

@harmbrugge harmbrugge harmbrugge approved these changes

@svandenhoek svandenhoek Awaiting requested review from svandenhoek

@joerivandervelde joerivandervelde Awaiting requested review from joerivandervelde

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: include refback columns in CSV output
4 participants
@mswertz @hslh @harmbrugge @connoratrug