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
Could not find attachment
fix with filenames which contain non-latin characters.
#4811
Conversation
e803ad5
to
0195a7a
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 Python code is fine to merge as-is, although I left one comment about a simplification of the LazyObject
subclass into an instance of SimpleLazyObject
👌
@@ -18,6 +19,14 @@ def get_kobocat_storage(): | |||
return KobocatFileSystemStorage() | |||
|
|||
|
|||
class DefaultKobocatStorage(LazyObject): |
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.
Interesting that this is only quasi-documented by Django. Seems safe to use, though 👍
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.
On second glance, the docstring for LazyObject
says:
By subclassing, you have the opportunity to intercept and alter the
instantiation. If you don't need to do that, use SimpleLazyObject.
Would this suffice?
default_kobocat_storage = SimpleLazyObject(get_kobocat_storage)
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.
FYI: I've replicated what Django is doing to instantiate default_storage
.
But your solution is great. It's "Simple" and there is less code. 💯
…x/kpi into fix-could-not-find-attachment
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.
Thank you! Requested one small change and left a general comment.
kpi/models/asset.py
Outdated
v = self.latest_deployed_version if deployed else self.latest_version | ||
survey = v.to_formpack_schema()['content']['survey'] |
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 can mostly tell what this function is doing right now, so I don't think it's a huge deal, but adding (brand new) one-letter variables is a little scary. 😅
Notes
JS uses the new added property
question_xpath
from data endpoint response to retrieve the correct attachment (instead of the attachment filename).Back-end should only try to add the property on the fly it is missing (see kobotoolbox/kobocat#915). It should cover existing data.
Related issues
Related to #3659
Related to kobotoolbox/kobocat#915