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.

Sign up for GitHub

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

Jump to bottom

Show current source document #1815

Merged
merged 1 commit into from
Aug 22, 2023
Merged

Show current source document #1815

merged 1 commit into from
Aug 22, 2023

Conversation

actlikewill
Copy link
Contributor

@actlikewill actlikewill commented Aug 18, 2023 •
edited
Loading

  • adds a view to convert docx to pdf upon request
  • shows docx attachment as source file by default

https://www.loom.com/share/d51ccc6882a742c5ae2bcb8f3b55334d?sid=26e9ecf0-da59-44c2-95fb-fd410f297719

Fixes #1786

@actlikewill actlikewill marked this pull request as draft August 18, 2023 11:53
@actlikewill actlikewill marked this pull request as ready for review August 18, 2023 13:08
@actlikewill
Copy link
Contributor Author

@longhotsummer @goose-life We could simplify this to handle everything in the /view view. and then in the js we could check for the first pdf attachment and show that by default. Let me know if we should do it like that instead.

goose-life
goose-life previously approved these changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@goose-life goose-life left a comment

Choose a reason for hiding this comment

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

looks great to me as is, other than those javascript errors, which seem to be just a syntax issue?

@goose-life
Copy link
Contributor

but for context, I did see this working earlier — maybe a video would help as part of the PR?

longhotsummer
longhotsummer reviewed Aug 21, 2023
View reviewed changes
indigo_app/static/javascript/indigo/views/document_source_att.js Outdated
@@ -61,11 +70,18 @@
choices = this.attachments.filter(function(att) {
return self.mime_types[att.get('mime_type')];
}).map(function(att) {
let url = att.get('view_url');
let default_choice;
if (Object.keys(self.docx_mimetypes).includes(att.get('mime_type'))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (Object.keys(self.docx_mimetypes).includes(att.get('mime_type'))) {
if (Object.keys(self.docx_mimetypes).includes(att.get('mime_type'))) {

actlikewill
actlikewill commented Aug 22, 2023
View reviewed changes
indigo_app/static/javascript/indigo/views/document_source_att.js
'application/vnd.oasis.opendocument.text': true,
'application/rtf' : true,
'text/rtf': true,
};
Copy link
Contributor Author

@actlikewill actlikewill Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@longhotsummer I'm happy to have this list here because I think the logic is more clear what we are doing. i.e we want the first document that is of these mimetypes only and nothing else

Copy link
Contributor

Choose a reason for hiding this comment

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

cool

actlikewill
actlikewill commented Aug 22, 2023
View reviewed changes
indigo_api/views/attachments.py
Comment on lines +72 to +73
if attachment.mime_type in DOC_MIMETYPES:
return view_attachment_as_pdf(attachment)
Copy link
Contributor Author

@actlikewill actlikewill Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@longhotsummer Similar to the reasoning in the js, I think this logic describes our goals a bit better i.e we are interested in converting docx types only rather than putting everything through soffice.

actlikewill
actlikewill commented Aug 22, 2023
View reviewed changes
indigo_api/views/attachments.py
Comment on lines +17 to +23
DOC_MIMETYPES = [
"application/vnd.oasis.opendocument.text",
"application/vnd.openxmlformats-officedocument.wordprocessingml.document",
"application/msword",
"application/rtf",
"text/rtf",
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@longhotsummer maybe this constant deserves a spot in docpipe itself?

@actlikewill
Copy link
Contributor Author

@goose-life Here is a loom for this: https://www.loom.com/share/d51ccc6882a742c5ae2bcb8f3b55334d?sid=26e9ecf0-da59-44c2-95fb-fd410f297719

longhotsummer
longhotsummer approved these changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@longhotsummer longhotsummer left a comment

Choose a reason for hiding this comment

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

super

@actlikewill actlikewill merged commit 191447a into master Aug 22, 2023
6 checks passed
@actlikewill actlikewill deleted the pubdoc branch August 22, 2023 12:15
@sentry-io
Copy link

sentry-io bot commented Aug 22, 2023 •
edited
Loading

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ FileNotFoundError: [Errno 2] No such file or directory: 'soffice' /api/documents/{document_id}/view View Issue
  • ‼️ SOfficeError /api/documents/{document_id}/view View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goose-life goose-life goose-life approved these changes

@longhotsummer longhotsummer longhotsummer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

As an editor / reviewer I don't want to compare an imported consolidation docx against the publication PDF
3 participants
@actlikewill @goose-life @longhotsummer