-
Notifications
You must be signed in to change notification settings - Fork 81
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
Fix #935: Conditional attributes not exported during download process #1083
Conversation
@oliverroick: I haven't actually started reviewing this PR since I'm doing my own unsolicited review of PR #1076. But I hope reviewing that PR helps me understand this PR. |
8e97c74
to
3217687
Compare
e1e27f1
to
54ce28e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The export to XLS doesn't seem to work. I had a project with a standard questionnaire and some data. Exporting to shape works, all information is in the SHP and the CSV files. Exporting to XLS however returns empty spreadsheets; both for data and resources. (This could also be a problem with Apple Numbers; I don't have Excel on my machine).
@linzjax Eugene is at a conference these days. Would you be able to jump in for the second review so we can get this sorted timely? |
I did already start reviewing this PR but nothing weird jumped out for me so far. My biggest observation is that I will need to base my search export feature code on this branch. Otherwise I'd have to wrangle with rebasing my work if I based it on the current master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished looking at the code changes and aside from a single comment, I didn't find anything wrong. Note also that I didn't test the download files this update is producing in my VM.
@@ -114,6 +114,14 @@ def get_model_attributes(self, project, content_type): | |||
attributes_for_models = self.get_attributes(project) | |||
return attributes_for_models[content_type] | |||
|
|||
def get_conditional_selector(self, content_type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is an example of how I feel the code seems to currently cater to the party entity being the only one right now to have a conditional selector. Maybe it is a good database design to have the conditional selector be a field of the model instead of a foreign field, but I kinda feel iffy with basing the decision on whether we have a conditional selector hinge on the presence of a period on the last schema selector.
Note that this is just an observation and should not be a blocker for the PR's approval.
Proposed changes in this pull request
Fix #935: Conditional attributes not exported during download process
This PR adds support for the export of conditional attributes. It makes use of the
SchemaSelectorMixin
added in PR #1076.openpyxl
to version2.4.1
write_items
methods ofShapeExporter
andXLSExporter
to export all attributesmake_download
method ofXLSExporter
to use a streamingWriteOnlyWorksheet
which streams rows to the spreadsheet rather than holding all data in memory before writing. This should help with improvements proposed in the Asynchronous Import/Export DRget_values
method of the baseExporter
to return a dict of values instead of a listget_conditional_selector
to theSchemaSelectorMixin
to return the conditional selector for a particularContentType
if one existsWhen should this PR be merged
This has been rebased from #1076 so should be merged after this.
Risks
Low risk.
Follow up actions
Reprovision VM's to pull in updates to dependencies.
Checklist (for reviewing)
General
migration
label if a new migration is added.Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.
Functionality
Code
Tests
Documentation