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.

Sign up for GitHub

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

Jump to bottom

Improve CSV download for Fit cohort view #27608

Merged
merged 3 commits into from Mar 23, 2019
Merged

Improve CSV download for Fit cohort view #27608

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8392c5a
Add more fields to Fit cohort CSV file. Sanitize content so it can be…
hacodeorg Mar 20, 2019
732846f
Fix PR comments, add unit tests
hacodeorg Mar 22, 2019
955ad0d
fix applications_controller unit test
hacodeorg Mar 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 26 additions & 2 deletions apps/src/code-studio/pd/application_dashboard/admin_cohort_view.jsx
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ import Select from 'react-select';
import $ from 'jquery';
import downloadCsv from '../downloadCsv';
import AdminCohortViewTable from './admin_cohort_view_table';
import _ from 'lodash';

const styles = {
downloadCsvButton: {
Expand Down Expand Up @@ -63,6 +64,18 @@ export default class AdminCohortView extends React.Component {
}
}

sanitizeStringForCsv = str => {
let res = str;
if (str) {
// Convert line breaker to dot, multiple spaces to single space
res = res.replace(/\n|\r\n|\r+/gm, '.');
Copy link

Choose a reason for hiding this comment

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

Let's put a space after the period to make it easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a bug, thanks for catching it

res = res.replace(/\s+/gm, ' ');
res = res.trim();
}

return res;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this, it is much cleaner!


handleDownloadCsv = () => {
const headers = {
date_accepted: 'Date Accepted',
Expand All @@ -72,6 +85,8 @@ export default class AdminCohortView extends React.Component {
email: 'Email',
assigned_workshop: 'Assigned Workshop',
registered_workshop: 'Registered Workshop',
assigned_fit: 'Assigned FiT',
registered_fit_submission_time: 'Registered FiT Submission Time',
accepted_seat: 'Accepted Seat?',
course_name: 'Course',
regional_partner_name: 'Regional Partner',
Expand All @@ -91,14 +106,23 @@ export default class AdminCohortView extends React.Component {
// Make sure we include all form_data keys that appear on any row:
Object.keys(row.form_data).forEach(formDataHeader => {
if (!headers[formDataHeader]) {
// Use the raw formData key as the column header
headers[formDataHeader] = formDataHeader;
// Convert formData key to more readable format, use it as the column header
headers[formDataHeader] = _.startCase(formDataHeader);
}
});

return {...row, ...row.form_data};
});

// Sanitize string content before exporting it to CSV
filteredCohortWithFormData.forEach(row => {
Object.keys(row).forEach(key => {
if (_.isString(row[key])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not typeof row[key] === 'string'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

row[key] = this.sanitizeStringForCsv(row[key]);
}
});
});

downloadCsv({
data: filteredCohortWithFormData,
filename: `${this.props.route.cohortType.toLowerCase()}_cohort.csv`,
Expand Down
9 changes: 7 additions & 2 deletions dashboard/app/serializers/api/v1/pd/fit_cohort_view_serializer.rb
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@ class Api::V1::Pd::FitCohortViewSerializer < ActiveModel::Serializer
:assigned_fit,
:registered_fit,
:accepted_fit,
:registered_fit_submission_time,
:role,
:status,
:locked,
Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

it would be great if we could make this a friendlier format to read, like we do in the timestamp log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think we should do it at the server side or client side? Should it be in UTC or local time zone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


def fit_assigned_at_registration
object.try(FIT_WEEKEND_REGISTRATION_FACTORY).try(:fit_city)
object.try(FIT_WEEKEND_REGISTRATION_SYMBOL).try(:fit_city)
end

def accepted_fit
object.try(FIT_WEEKEND_REGISTRATION_FACTORY).try(:accepted_seat_simplified)
object.try(FIT_WEEKEND_REGISTRATION_SYMBOL).try(:accepted_seat_simplified)
end

def role
Expand Down