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

#2714 batch status change #2744

Merged
merged 20 commits into from
Jul 7, 2022
Merged

#2714 batch status change #2744

merged 20 commits into from
Jul 7, 2022

Conversation

iamleeg
Copy link
Contributor

@iamleeg iamleeg commented Jul 6, 2022

Ready for review. My problem was not deserialising a model object correctly so I've introduced a superclass to make it more obvious what custom serialisation/deserialisation code a particular document type supplies.

Copy link
Contributor

@abhidg abhidg left a comment

Choose a reason for hiding this comment

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

looks good, other than a typo


def to_csv(self) -> str:
"""Generate a row in a CSV file representing myself."""
return self.delimiter_separated_values(",")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this handle commas in values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, no, not yet…that's a good case to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The challenge is though that there's nowhere a comma can appear in the model (yet) such that it will appear in the CSV file. We don't want a free-form notes field for privacy reasons, and exclusion metadata is not included in the CSV download (as it isn't included in the data-service download either). So while there's a reason to do this, there isn't currently a situation where it can occur. I suggest merging this PR and revisiting it when the DayZeroSchema or custom schema includes a field that can have a comma in.

@abhidg
Copy link
Contributor

abhidg commented Jul 7, 2022 via email

@iamleeg iamleeg merged commit 79a3d30 into main Jul 7, 2022
@iamleeg iamleeg deleted the 2714_batch_status_change branch July 7, 2022 15:30
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.

None yet

2 participants