-
Notifications
You must be signed in to change notification settings - Fork 0
#140: Fixed a bug that hindered download of large amounts of study data #6
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
#140: Fixed a bug that hindered download of large amounts of study data #6
Conversation
…s of study data Also extracted test settings into a dedicated vitest.config and changed env variable to let user change the backend url more easy
| await importExportApi | ||
| .exportStudyData( | ||
| studyId, | ||
| rs.data.token, | ||
| studyGroupId, | ||
| participantId, | ||
| observationId, | ||
| from, | ||
| to, | ||
| { responseType: 'blob' }, | ||
| ) | ||
| .then((response) => { | ||
| const blob = response.data as unknown as Blob; | ||
|
|
||
| }).catch((e: AxiosError) => { | ||
| handleIndividualError( | ||
| e, | ||
| 'cannot export data despite of existing download token', | ||
| ); | ||
| } ); | ||
| const url = URL.createObjectURL(blob); | ||
| const link = document.createElement('a'); | ||
| link.href = url; | ||
| link.download = `study_data_${studyId}.json`; | ||
| document.body.appendChild(link); | ||
| link.click(); | ||
| document.body.removeChild(link); | ||
| URL.revokeObjectURL(url); | ||
| }) | ||
| .catch((e: AxiosError) => { | ||
| handleIndividualError( | ||
| e, | ||
| 'cannot export data despite of existing download token', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soviel isch sehe ist das der Fix für den Bug. Alle anderen Änderungen haben nichts mit dem #6 zu tun, oder?
Es wäre echt besser, wenn Du einfach 2 Merge Requests aufmachst.
- Den ersten für das Refactoring - auch wenn da dann kein Issue mit dabei ist.
- Den zweiten mit nur den Fix für den Bug im Issue!
So schaue ich mir einen MR über 9 Files mit hunderten Zeilen an Änderungen an, aber der eigentliche fix sind nur 20 Zeilen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ja es hat sonst leider bei mir immer Probleme gemacht, weswegen ich leider alles andere auch ändern musste.
| const url = URL.createObjectURL(blob); | ||
| const link = document.createElement('a'); | ||
| link.href = url; | ||
| link.download = `study_data_${studyId}.json`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wäre es nicht besser, wenn das Webservice schon einen passenden Content-Disposition Header setzten würde. z.B.
Content-Disposition: attachment; filename="study_data_study-{id}.json"
Würde in den Namen auch gleich noch andere Filter wie StudyGroup oder Participant sowie die Time-Range mit reincodieren (am Backend). Dieser Header sollte dann den Browser dazu veranlassen das Ergebnis direkt in einem File zu speichern
westei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Als Bugfix OK, aber @janoliver20 bitte erstelle einen Issue, dass wir das auf den Content-Disposition im Backend und in der OpenAPI Spec umstellen. Den Issue können wir dann ja in einem Sprint einplanen, wenn wir am StudyManager Backend was machen
Also extracted test settings into a dedicated vitest.config and changed env variable to let user change the backend url more easy