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

Create CSV export with only selected query rows #3476

Merged
merged 17 commits into from
May 24, 2023
Merged

Conversation

CarolineDenis
Copy link
Contributor

Fixes #2348

@maxpatiiuk
Copy link
Member

@CarolineDenis since this branch was based on specify-network-integration, you should pick it as your pull request merge target rather than production

@CarolineDenis CarolineDenis changed the base branch from production to specify-network-integration May 10, 2023 15:13
@specify specify deleted a comment from maxpatiiuk May 15, 2023
Triggered by 4062ef9 on branch refs/heads/issue-2348
@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented May 16, 2023

@maxpatiiuk @CarolineDenis Can this change be transferred to production as we have no timeline for release yet for specify-network-integration? That may mean creating a new PR

@maxpatiiuk
Copy link
Member

@grantfitzsimmons
It's on this branch specifically because this branch modified the relevant code
specify network integration can be released as soon as it's tested - it has all the features I wanted it to have for v1
the collection-wide gbif maps are also there, but they are not linked to from anywhere yet

@CarolineDenis CarolineDenis marked this pull request as ready for review May 17, 2023 13:02
@CarolineDenis CarolineDenis requested a review from maxpatiiuk May 17, 2023 13:02
@CarolineDenis CarolineDenis marked this pull request as draft May 17, 2023 15:06
@CarolineDenis CarolineDenis requested a review from maxpatiiuk May 17, 2023 21:06
Triggered by f73d716 on branch refs/heads/issue-2348
Base automatically changed from specify-network-integration to production May 19, 2023 15:26
@CarolineDenis CarolineDenis marked this pull request as ready for review May 19, 2023 20:44
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

After thinking futher about this code, I do like your solution of making selectedRows accessible to parent components as this might even be useful for other use cases in the future.

The only advice I have here is to change resultsArray from being a state to being a ref. Please think though implications of this change (i.e, what's the difference between state and ref, especially when it comes to triggering react re-renders) and why it's beneficial to do that change in here

@CarolineDenis CarolineDenis requested a review from maxpatiiuk May 22, 2023 18:56
Triggered by 428eb63 on branch refs/heads/issue-2348
@carlosmbe
Copy link
Contributor

Works. But exported files with only selected rows were .tsv
Not sure if collection managers are picky about that but maybe change the button to Create TSV in those situations

@CarolineDenis CarolineDenis requested review from maxpatiiuk and a team May 22, 2023 19:57
Triggered by be660f5 on branch refs/heads/issue-2348
@maxpatiiuk
Copy link
Member

Works. But exported files with only selected rows were .tsv Not sure if collection managers are picky about that but maybe change the button to Create TSV in those situations

Good catch! The code should not be hardcoded to use .tsv extension - you should either use .csv (which is a universal file extension for this type of data), or have a different file extension depending on the delimiter used (.tsv - tab, .csv - comma, .psv - pipe, and others)

The button however should always say Create CSV as CSV is far more recognizable to an average computer user than more obscure formats like TSV or PSV

@carlosmbe
Copy link
Contributor

Works. But exported files with only selected rows were .tsv
Not sure if collection managers are picky about that but maybe change the button to Create TSV in those situations

Also the naming format of the exported files differs:

query_results_2023-05-22T20_21_21.052320.csv

vs

taxon query - Mon May 22 2023.tsv

@CarolineDenis CarolineDenis requested a review from maxpatiiuk May 22, 2023 20:27
Triggered by ae58862 on branch refs/heads/issue-2348
Copy link
Contributor

@carlosmbe carlosmbe left a comment

Choose a reason for hiding this comment

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

Mainly related to maintaining consistency with the old Create CSV
Two things:
1- The old Create CSV lets users download the file through the notifications while selected query are automatically downloaded.
2- Naming format of the exported files still differs from the old Create CSV

@maxpatiiuk
Copy link
Member

Yes, we should improve the back-end file name. But, Caroline created a separate issue for that, so it doesn't necessarily have to be fixed as part of this PR - #3525

As far as using notifications:

  1. The notifications system was used due to technical limitations (because it needs time to fetch all query results and generate the file) - here there is no reason to wait because the query results are already fetched (because user can only select query results that are fetched).
  2. Front-end has not technical capability to send someone notifications, so we can't use that - only back-end has the power to create notifications at the moment.

Thus, this inconsistency is justified

@CarolineDenis CarolineDenis merged commit 6941cec into production May 24, 2023
@CarolineDenis CarolineDenis deleted the issue-2348 branch May 24, 2023 15:51
@bronwyncombs
Copy link

v7.8.12-prerelease : The Create CSV button adds selected rows only to the download file. The file name format is Query Name - Day Date.

@melton-jason
Copy link
Contributor

@bronwyncombs can you expand upon this please? What exactly did you find while testing this?
Can you describe what you were doing (so we can recreate an issue)?
Screenshots, Videos, or direct links are always helpful for this process as well!
Essentially, try and format your response like an Issue (if you don't know how issues are formatted, check out any of them from the Issues tab!).
Because of this, it may be easier to open an Issue and link the associated Pull Requests (or other Issues) with # and then entering the ID or the name.

I tested this on v7.8.12-prerelease and here are my results.

When exporting a query to CSV that has not been saved, it exports correctly (only the selected rows).
However, there was an issue related to the name of the Query (most likely related to #3525) .

The Query name is New Query -_ Collection Object - Fri Jun 02 2023.csv.
There seems to be extra space characters.
new_co_query

New Query -_ Collection Object - Fri Jun 02 2023.csv

Exporting a saved Query seems to work as intended. The selected rows export properly, and the file is named correctly.

saved_co_query

CO Query - Fri Jun 02 2023.csv

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.

Query builder's Export to CSV ignores selected rows
6 participants