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

record-manager-ui#36 Implement export filtered records to excel #52

Merged

Conversation

kostobog
Copy link

@kostobog kostobog commented Jul 9, 2024

kostobog added 4 commits July 9, 2024 17:19
- add sparql query
- add export related entities
- add dao layer code to retrieve exported record data
- add ExcelRecordExporter service exporting records to excel
@kostobog kostobog requested a review from blcham July 9, 2024 16:11
@GetMapping(value = "/export", produces = MediaType.APPLICATION_JSON_VALUE)
public List<PatientRecord> exportRecords(
@GetMapping(value = "/export", produces = {MediaType.APPLICATION_JSON_VALUE, Constants.MEDIA_TYPE_EXCEL})
public ResponseEntity<?> exportRecords(
Copy link

Choose a reason for hiding this comment

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

We just need to make sure that we did not change implementation of JSON export.

Copy link

Choose a reason for hiding this comment

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

@LaChope please test it when implementing UI

Copy link
Collaborator

Choose a reason for hiding this comment

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

I exported twice, before and after this PR, output JSON file is similar

/**
* Maps the specified parameters to a new {@link RecordFilterParams} instance.
*
* @param params Request parameters to map
* @return New {@code RecordFilterParams} instance
*/
public static RecordFilterParams constructRecordFilter(MultiValueMap<String, String> params) {
public static RecordFilterParams constructRecordFilter(final RecordFilterParams result, MultiValueMap<String, String> params) {
Copy link

Choose a reason for hiding this comment

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

Not sure why we need this.

Copy link

Choose a reason for hiding this comment

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

This is here as we did not want to include into query MIN_MODIFIED_DATE and MAX_MODIFIED_DATE .. this should be modified to remove.

Copy link

Choose a reason for hiding this comment

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

DO NOT FIX NOW.

/**
* Maps the specified parameters to a new {@link RecordFilterParams} instance.
*
* @param params Request parameters to map
* @return New {@code RecordFilterParams} instance
*/
public static RecordFilterParams constructRecordFilter(MultiValueMap<String, String> params) {
public static RecordFilterParams constructRecordFilter(final RecordFilterParams result, MultiValueMap<String, String> params) {
Copy link

Choose a reason for hiding this comment

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

This is here as we did not want to include into query MIN_MODIFIED_DATE and MAX_MODIFIED_DATE .. this should be modified to remove.

/**
* Maps the specified parameters to a new {@link RecordFilterParams} instance.
*
* @param params Request parameters to map
* @return New {@code RecordFilterParams} instance
*/
public static RecordFilterParams constructRecordFilter(MultiValueMap<String, String> params) {
public static RecordFilterParams constructRecordFilter(final RecordFilterParams result, MultiValueMap<String, String> params) {
Copy link

Choose a reason for hiding this comment

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

DO NOT FIX NOW.

kostobog added 3 commits July 10, 2024 10:54
… when extracting exportType from request. exportType uri parameter takes precedence over Accept header.
…nstitution request parameter.

- remove unnecessary annotations from method parameters
@kostobog kostobog force-pushed the feature/record-manager-ui-36-export-filtered-records-to-excel branch from d4a230e to 0b985cd Compare July 10, 2024 09:39
@kostobog
Copy link
Author

@blcham
One more thing, Excel export exports all records matching the filter in the query parameters but it ignores the pagination parameters. Is this ok?

@blcham
Copy link

blcham commented Jul 10, 2024

@blcham One more thing, Excel export exports all records matching the filter in the query parameters but it ignores the pagination parameters. Is this ok?

It should behave same as for export to JSON, so if it ignores it as well then it is ok. (you can even test on kbss :)

@kostobog
Copy link
Author

@blcham
Paging support for excel export is implemented

@blcham blcham merged commit 4ec18b7 into main Jul 10, 2024
2 checks passed
@blcham blcham deleted the feature/record-manager-ui-36-export-filtered-records-to-excel branch July 10, 2024 13:30
UriComponentsBuilder uriBuilder, HttpServletResponse response) {
@RequestParam(required = false) MultiValueMap<String, String> params,
UriComponentsBuilder uriBuilder, HttpServletRequest request, HttpServletResponse response) {
MediaType exportType = Stream.of(
Copy link

Choose a reason for hiding this comment

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

@LaChope here the way how to differentiate between JSON and EXCEL

Either use accept header or parameter in the query string:

public static final String EXPORT_TYPE_PARAM = "exportType";
public static final String MEDIA_TYPE_EXCEL = "application/vnd.ms-excel"

Copy link

@blcham blcham Jul 10, 2024

Choose a reason for hiding this comment

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

Thus you need to use alternatively following query parameters:
&exportType=application/vnd.ms-excel
&exportType=application/json

Copy link
Collaborator

Choose a reason for hiding this comment

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

@blcham @kostobog Not sure that is works:

image

Copy link

Choose a reason for hiding this comment

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

ok, I'm not sure why it does not work, but you can try the exportType query parameter instead of the Accept header.

Copy link
Author

@kostobog kostobog Jul 10, 2024

Choose a reason for hiding this comment

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

@blcham @LaChope
Record paging implementation does not support sorting without the page parameter. Try to remove the sort parameter or add a page parameter.

Copy link
Collaborator

@LaChope LaChope Jul 10, 2024

Choose a reason for hiding this comment

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

Removing the sort parameter fixed it. I can try with the page parameter but I do not know what does it represent? The number of sheet?

Copy link
Author

@kostobog kostobog Jul 10, 2024

Choose a reason for hiding this comment

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

@LaChope
The page parameter represents the index of the page. The page has size number of records. (request parameter, default is 25 records per page).

I am not sure what are you trying to do. There are three two things that I think makes sense to implement.

  1. Export all records matching the filter specified by the user. I think the user specifies the filter using the table.
  • In this case the request should contain the same filter, e.g. min max date, institution, as the one used to filter the records in the table. page, size and sort parameters should be omitted.
  1. Export the records on the current page.
  • In this case the request should contain the filter parameters as well as the and the page, size, and sort parameters.

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.

3 participants