-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added support for export of records with MSExcel #178
Conversation
src/actions/RecordsActions.js
Outdated
@@ -61,9 +61,6 @@ export function exportRecords(exportType, params = {}) { | |||
.get(`${API_URL}/rest/records/export`, { | |||
params, | |||
paramsSerializer, | |||
headers: { |
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.
I removed this as described here: kbss-cvut/record-manager#52 (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.
I suggest to revert this change and keep the accept header.
Without it the backend will not receive the export type and it will always return 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.
No, it returns the correct file type even without it (not sure how tbh, I think it it because of the file extension), but if I add the headers, the backend returns 406 (only for excel file types)
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.
@LaChope
I just tested this PR. The when downloading excel the file has xslx
extension but the content is 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.
Oohh ok I see, then I can put back the headers but I will get 406 error.
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.
@kostobog I reverted
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.
@@ -104,6 +104,7 @@ class RecordsController extends React.Component { | |||
this.props.exportRecords(exportType, { | |||
...this.state.filters, | |||
sort: sortToParams(this.state.sort), | |||
page: this.state.pageNumber, |
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.
@kostobog I believe this is what this parameter is supposed to represent?
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.
@LaChope
Yes I think this the right parameter. You should also set the size parameter, see how it is done in _loadRecords()
.
@kostobog please revise |
@blcham @LaChope |
No description provided.