-
Notifications
You must be signed in to change notification settings - Fork 62
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
652: added Document (file) media type #668
652: added Document (file) media type #668
Conversation
79b07ed
to
6676ac7
Compare
6676ac7
to
121a17d
Compare
Signed-off-by: Vinicius Cubas Brand <[email protected]>
121a17d
to
3b7ca5f
Compare
Thanks for these PRs! Hoping to get to them next week. In the meantime, if you could resolve any merge conflicts (preferably with a rebase), that would be helpful! |
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.
Nice work! Very thorough! Thanks for taking the time to go through the whole codebase and cover all areas.
I have questions about ODK support. Let me know if that's been tested. If it has, wow, surprise for me! If it hasn't, I'm pretty sure it's not supported, so let's take it out and add a spec to ensure document questions aren't rendered for ODK. See the form_rendering_spec.rb
file.
@@ -127,6 +127,7 @@ def media_type | |||
when "image", "annotated_image", "sketch", "signature" then "image/*" | |||
when "audio" then "audio/*" | |||
when "video" then "video/*" | |||
when "document" then "document/*" |
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.
Has this been tested in ODK Collect? What does it do? I know that image, audio, and video have specific meanings in Collect but I haven't heard of "document" in that context. If it's not supported by ODK then we should not be rendering this question type when rendering for Collect, and this deliberate omission should be tested in the form_rendering_spec.rb
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.
Hi @smoyth. Yes, we've tested in ODK collect and also in Nemo web interface. I've tested by uploading a document (pdf, odt, doc) in the collect, the answer was saved and after I could download/preview that document in the answer screen. This also worked in the web interface.
application/vnd.oasis.opendocument.presentation | ||
application/x-ole-storage].freeze | ||
ODK_MEDIA_EXTS = {audio: %w[mp3 ogg wav flac], video: %w[mp4], image: %w[png jpg jpeg], | ||
document: %w[pdf xls xlsx ods doc docx odt rtf ppt pptx odp csv txt]}.freeze |
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.
If ODK doesn't support the document type then all of this can be removed, yes?
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", | ||
"application/vnd.openxmlformats-officedocument.wordprocessingml.document", | ||
"application/vnd.openxmlformats-officedocument.presentationml.presentation", | ||
# sometimes mimemagic returns x-ole-storage for msoffice files: https://github.com/minad/mimemagic/issues/50 |
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.
Thanks for the comment!
@@ -75,6 +75,10 @@ class QuestionType | |||
name: "video", | |||
odk_name: "binary", | |||
properties: %w[multimedia] | |||
}, { | |||
name: "document", | |||
odk_name: "binary", |
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.
Maybe don't need this?
# upload valid document | ||
drop_in_dropzone(document, 2) | ||
expect_preview(document_node) | ||
|
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.
Nice specs!
@@ -0,0 +1,2 @@ | |||
This is a document., | |||
"Answer to the Ultimate Question of Life, the Universe, and Everything:",42 |
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.
😆
</text> | ||
<text id="*itemcode4*:hint"> | ||
<value>Question Hint 4</value> | ||
</text> |
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.
Curious if this works too. In the docs, I only see support for image, audio, and photo media prompts:
@@ -4,7 +4,7 @@ module Odk | |||
class QuestionDecorator < ApplicationDecorator | |||
delegate_all | |||
|
|||
URI_DIRS_BY_TYPE = {video: "video", audio: "audio", image: "images"}.freeze | |||
URI_DIRS_BY_TYPE = {video: "video", audio: "audio", image: "images", document: "document"}.freeze |
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.
If ODK document media prompts aren't supported then this should be removed too.
cfd1f24
to
cf84400
Compare
74cb6eb
to
62b3d8d
Compare
Closing for now due to age. Can be opened again later. |
Proposal for a Document media type.
Resolves #652