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

dev/core#4031 Export dates from SearchKit to spreadsheet in SQL format #26211

Merged

Conversation

larssandergreen
Copy link
Contributor

Before

From SearchKit Download Spreadsheet, dates are formatted in Complete Date and Time format. The default for this format is not readily recognized as a date in spreadsheets.

After

Dates are formatted in SQL DATETIME or DATE format for csv, xlxs and ods downloads. No change in pdfs, SearchKit itself or for dates that have been re-written in a Display.

@civibot
Copy link

civibot bot commented May 13, 2023

No issue was found matching the number given in the pull request title. Please check the issue number.

@civibot
Copy link

civibot bot commented May 13, 2023

(Standard links)

@civibot civibot bot added the master label May 13, 2023
@colemanw
Copy link
Member

This is definitely the functionality we want, but I'm concerned the code might be better placed in Civi\Api4\Action\SearchDisplay\Download::formatColumnValue() as that's more specific to the Download action.

@larssandergreen
Copy link
Contributor Author

@colemanw I hear you, that would be cleaner. But by the time we're in formatColumnValue() the date is already formatted, so I think we have to go back to the point where it is in SQL format, rather than trying to work backwards from the formatted value. Or we could pass the raw value and data type along from AbstractRunAction::formatColumn(), so we're able to use it in formatColumnValue(), but that seems like a lot just for the rare case when we are downloading a spreadsheet with dates.

But it's not unlikely that I'm missing an obvious cleaner option - welcome any thoughts you have on a better approach.

@larssandergreen
Copy link
Contributor Author

@colemanw How about something like this that keeps it all in Download?

@larssandergreen
Copy link
Contributor Author

I'll update the tests if this makes sense.

* @return array|string
*/
protected function formatViewValue($key, $rawValue, $data) {
protected function formatViewValue($key, $rawValue, $data, $dataType = NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this new param $dataType? I don't see it ever being passed into either this function nor Download:: formatViewValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is so that Download::formatViewValue can pass the result of $this->getSelectExpression($key)['dataType'] to AbstractRunAction::formatViewValue, so we don't have to do it twice for every row we download. Just thought if we're downloading 10k rows, maybe we want to avoid redundant steps.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, following the code to $this->getSelectExpression($key) it looks pretty efficient, it's just grabbing the value from a cached array. I don't think you're gaining any performance there, but I could see it as a code simplification if you make the $dataType param required and do not look it up within this function at all.

@larssandergreen larssandergreen force-pushed the export-dates-from-SK-in-ISO-format branch from f1e0449 to b30ce8d Compare May 17, 2023 23:25
@larssandergreen larssandergreen force-pushed the export-dates-from-SK-in-ISO-format branch from b30ce8d to dbc32dc Compare May 18, 2023 00:06
@colemanw
Copy link
Member

Current revision looks great. Thanks for your work on this @larssandergreen - nice job!

@larssandergreen
Copy link
Contributor Author

Great, thanks @colemanw for your help on this.

@colemanw colemanw merged commit c7dd67b into civicrm:master May 18, 2023
3 checks passed
@larssandergreen larssandergreen deleted the export-dates-from-SK-in-ISO-format branch May 24, 2023 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants