PDF-export by work packages query (backend only) #12312
Conversation
* overview table * work package details * batching removed (for now) * attachments table removed, export inlined images in description instead
|
Hi @as-op, Thanks a lot for issuing this preview. Very helpful. Best |
|
Hi @lindenthal, wow you are fast. :D It is fixed now. Best, Andrej |
# Conflicts: # Gemfile.lock
…nto the "non-dangling header before page break" check
as-op
changed the title
# Conflicts: # Gemfile.lock
as-op
changed the title
There was a problem hiding this comment.
First of all, with inline comments, I am trying to follow https://github.com/erikthedeveloper/code-review-emoji-guide. So everything that is not a 🔧 does not require attention from my point of view.
Code is straight forward and does what it's supposed to. PDF looks nice and seems to follow the requirements written in the ticket (for the portions where it is supposed to).
Personally, I am not the hugest fan of splitting code into seperate modules that require to be included together. This feels like it is splitting code up to follow a metric like ClassLength or AbcSize, but by requiring a developer to keep 6 files in mind/open to figure out where each method is coming from. So there is some potential to refactor this. I left some inline ideas how this might be done in the future. FWIW in this PR we are mostly following the existing structure and just extending it. In the chat with @as-op, they mentioned that there will be future refactorings, i.e. the styles will be loaded from YAML Content out of the DB in the future. Maybe there is some potential to do a first refactoring and instead of loading the styles as a bunch of included methods, have a PDFExportStyle class, that can be instantiated with the DB Content, extracts the data and then the xxx_style methods will be replaced by accessing that object.
📌 Also, the whole code is untested. In the past we have used the pdf-reader gem to write RSpec tests, that open the generated PDF and at least check that certain content we are expecting is rendered correctly. Right now it feels like it's mostly tested by visual inspection.
✅ approve from my side
| begin | ||
| draw_node(root, pdf_root_options(@styles.page), true) | ||
| rescue StandardError => e | ||
| Rails.logger.error "Failed to draw markdown pdf: #{e}" |
There was a problem hiding this comment.
❓ I guess this is for development only? Or is this the common error reporting behavior we are using? Write to the error log and then have the customer send us that log for investigation? Maybe a question to @ulferts or @oliverguenther
There was a problem hiding this comment.
Not really for development. My intention was to avoid that a PDF export with thousands of work packages would fail as whole if one description or long text field would somehow cause an error. E.g. when prawn can't fit a user inserted table on a page at all.
I should probably at least reduce the scope and catch Prawn::Errors::CannotFit only.
There was a problem hiding this comment.
Follow up: I did reduce the scope.
There was a problem hiding this comment.
If it's a background job, ideally it would report that to the job status so it becomes visible to the user in here: https://github.com/opf/openproject/blob/dev/app/workers/exports/export_job.rb#L118
Maybe keep that in mind as an implementation ticket to ensure errors get reported to the export modal, so this can get merged.
100% agreement. I'm not going to use the excuse that it was not tested before 😀, but the better explanation that |
https://community.openproject.org/projects/openproject/work_packages/46226/activity