Reactivate Japanese Locale #542
Conversation
cassidysymons
commented
Oct 23, 2023
cassidysymons
commented
- Reactivate the Japanese locale
- Update Japanese translations for surveys and consents
- Add an external reports framework that's used to ingest the THDMI Japan FFQs and associate them with the appropriate sources
| return jsonify(report.to_api()), 200 | ||
|
|
||
|
|
||
| def get_external_report_bytes( |
There was a problem hiding this comment.
| def get_external_report_bytes( | |
| def get_external_report_pdf( |
The MIME type is specific to PDF
There was a problem hiding this comment.
The MIME type is stored on a per-file basis and it's entirely possible it will be used for non-PDF file types in the future. Do you have a suggestion on an alternative to _bytes that's not specific to a single file type?
There was a problem hiding this comment.
bytes is fine but perhaps use the stored MIME type rather than hard code it on response?
| "file_contents, report_type" | ||
| ") VALUES (%s, %s, %s, %s, %s, %s)", | ||
| (source_id, pdf_name, JFFQ_FILE_LABEL, | ||
| "application/pdf", pdf_contents, "ffq") |
There was a problem hiding this comment.
It looks like the MIME type is stored along side the contents, why not pull this out for the _bytes API call?
There was a problem hiding this comment.
The file_contents value (the actual bytes of the file) don't go through the jsonify process cleanly, so I've separated the API calls to return the bytes in one call and the other information in another call.
| -- is for highly specific documents with a pre-defined language (FFQs from the THDMI Japan project), the approach makes sense. | ||
|
|
||
| -- The report_type value will dictate which section of the My Reports tab it displays under. | ||
| -- I'm setting the enum up to reflect current structure (sample = My Kits/ffq = My FFQs) but this could be extended to "Other" or various specifics later. |
There was a problem hiding this comment.
shouldn't this be 'kit', 'ffq' then as a sample is a component of a kit?
An enum of Other suggests we don't know what the entity is, and I think long term it would be better to avoid a catch all
There was a problem hiding this comment.
I used 'sample' because the existing reports are relative to a sample but listed under the My Kits section of the My Reports tab in the UI. Open to changing it if you feel strongly, though.
There was a problem hiding this comment.
That is a confusing item in the UI in retrospect. The enum looses value if it's not accurate, so suggest having it reflect the data relation
| self.source_id = kwargs['source_id'] | ||
| self.report_type = kwargs['report_type'] | ||
|
|
||
| # Optional parameters |
There was a problem hiding this comment.
Aren't these not null in the table spec?
| """Retrieve list of external reports for a given source, if any | ||
| NB: We only extract certain columns because there's no need to waste | ||
| resources on the actual file_contents until the user hits the path | ||
| to view/download the report |
There was a problem hiding this comment.
| """Retrieve list of external reports for a given source, if any | |
| NB: We only extract certain columns because there's no need to waste | |
| resources on the actual file_contents until the user hits the path | |
| to view/download the report | |
| """Retrieve list of external reports for a given source, if any | |
| We only extract certain columns because there's no need to waste | |
| resources on the actual file_contents until the user hits the path | |
| to view/download the report |
The cost of pulling this from the database is really small though, and introduces multiple paths to pull rows from this table, unless the plan is to store many MB or GB of data per row (which I do not believe is the case, and not what the design here is oriented toward). If the data isn't needed for the client, why not just have the API method delete it?
|
@wasade ready for re-review. Thanks! |
| file_contents: | ||
| type: string | ||
| report_type: | ||
| enum: ["sample", "ffq"] |
