Improve CSV download for Fit cohort view #27608
Conversation
… opened in spreadsheet tool.
| let res = str; | ||
| if (str) { | ||
| // Convert line breaker to dot, multiple spaces to single space | ||
| res = res.replace(/\n|\r\n|\r+/gm, '.'); |
There was a problem hiding this comment.
Let's put a space after the period to make it easier to read
| @@ -47,12 +48,16 @@ def registered_fit | |||
| object.try(:registered_fit_workshop?) | |||
| end | |||
|
|
|||
| def registered_fit_submission_time | |||
| object.try(FIT_WEEKEND_REGISTRATION_SYMBOL).try(:created_at) | |||
| end | |||
There was a problem hiding this comment.
it would be great if we could make this a friendlier format to read, like we do in the timestamp log.
There was a problem hiding this comment.
Do you think we should do it at the server side or client side? Should it be in UTC or local time zone?
| } | ||
|
|
||
| return res; | ||
| }; |
There was a problem hiding this comment.
Since both replace and trim return a new string instead of modifying the original string, you could chain these calls.
sanitizeStringForCsv = str => (str || '')
.replace(/\n|\r\n|\r+/gm, '.')
.replace(/\s+/gm, ' ')
.trim();There was a problem hiding this comment.
I like this, it is much cleaner!
| let res = str; | ||
| if (str) { | ||
| // Convert line breaker to dot, multiple spaces to single space | ||
| res = res.replace(/\n|\r\n|\r+/gm, '.'); |
There was a problem hiding this comment.
There's a bug here - did you mean (\n|\r\n|\r)+? As-is, you'll match multiple consecutive carriage returns \r but not multiple consecutive line breaks in other formats.
There was a problem hiding this comment.
it's a bug, thanks for catching it
| // Sanitize string content before exporting it to CSV | ||
| filteredCohortWithFormData.forEach(row => { | ||
| Object.keys(row).forEach(key => { | ||
| if (_.isString(row[key])) { |
There was a problem hiding this comment.
Why not typeof row[key] === 'string'?
There was a problem hiding this comment.
I learn that idea from here https://stackoverflow.com/a/9436948; it seems like the safe way is to do typeof myVar === 'string' || myVar instanceof String. I'm not fully confident with Javascript yet to know if there is any other corner case, so I opt to use a library instead. What do you think?
There was a problem hiding this comment.
Got it - using the helper is fine. In practice I think it's so rare for us to use new String() that the instanceof call isn't needed, but no complaints from me about covering all possibilities!
|
Add test coverage for
|
PLC-161
downloadCsvintoAdminCohortViewso the function can be tested properly.AdminCohortViewcomponentHow tested
Unit test
appsfolder, runyarn test:entry --entry test/unit/code-studio/pd/application_dashboard/admin_cohort_viewTest.jsdashboardfolder, runbundle exec rails test test/controllers/api/v1/pd/applications_controller_test.rbManual test