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.

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

Bump weasyprint #5885

Merged
merged 8 commits into from
Nov 19, 2023
Merged

Bump weasyprint #5885

merged 8 commits into from
Nov 19, 2023

Conversation

matmair
Copy link
Contributor

@matmair matmair commented Nov 8, 2023

This removes the pin as the issue that caused the pin seems to be resolved in django-waesyprint.
Might also close #4936, pending confirmation by @miggland

Closed #4936

@matmair matmair added the dependency Relates to a project dependency label Nov 8, 2023
@matmair matmair added this to the 0.13.0 milestone Nov 8, 2023
@matmair matmair self-assigned this Nov 8, 2023
Copy link

netlify bot commented Nov 8, 2023

Deploy Preview for inventree-web-pui-preview canceled.

Name Link
🔨 Latest commit 493877e
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/655691ee09b1d80008e4ecc4

@miggland
Copy link
Contributor

miggland commented Nov 9, 2023

This issue was blocking using an upgraded django-weasyprint: fdemmer/django-weasyprint#71 . The fix was merged (fdemmer/django-weasyprint#70), but not released?

Either way: if Weasyprint can be successfully upgraded, #4936 can be closed.

@matmair matmair modified the milestones: 0.13.0, 1.0.0 Nov 11, 2023
@matmair
Copy link
Contributor Author

matmair commented Nov 11, 2023

You are right @miggland, we will have to push this

@SchrodingersGat
Copy link
Member

The fix in django-weasyprint has actually been merged, but the release was not pushed to GitHub:

https://pypi.org/project/django-weasyprint/

@miggland
Copy link
Contributor

Well spotted @SchrodingersGat - but as of 13 hours ago, that's been corrected as well it seems!

@SchrodingersGat
Copy link
Member

@miggland yes I reached out to the maintainer of the project :)

@matmair matmair marked this pull request as ready for review November 12, 2023 13:15
@matmair matmair modified the milestones: 1.0.0, 0.13.0 Nov 12, 2023
@SchrodingersGat
Copy link
Member

It appears that the rendered PDF does not contain the exact same information. If you are able to test that the generated PDF files are still good, we can adjust the single failing test

@miggland
Copy link
Contributor

I have tried out (again) with the dev branch: SVGs don't show up.
With this branch, they look correct! All of the PDFs I've created in my test setup look fine. I used the standard demo data and made a few different labels/reports (also on sheets, or concatenated reports). The only thing I changed was to add an example with the SVG.

So: this seems to fix #4936, and looks good otherwise.

The test: I guess a more robust test would be to try and open the PDF with some tool (eg PikePDF), eg:
pdf = pikepdf.open(labelpath)
which gives exception pikepdf.PdfError if there is something wrong with PDF:

Alternative: pdftotext:

with open(labelpath, "rb") as f:
    pdf = pdftotext.PDF(f)
    filetext = " ".join(pdf)  # Join for multiple pages

But that will only work if there is actually text in the PDF..

Of course, both require installation of extra packages..

@matmair
Copy link
Contributor Author

matmair commented Nov 16, 2023

I am not sure adding a dependency is great but I also can not find a better solution, pretty much on the same page as @miggland.

I did not work with pikepdf - which is smaller than what I tried. I will adapt this PR to add a test based on it.

@matmair
Copy link
Contributor Author

matmair commented Nov 16, 2023

@miggland I added a co-author credit for you as the change is basically what you suggested above

@matmair
Copy link
Contributor Author

matmair commented Nov 16, 2023

The order of objects seems flaky between DBMS, a fix will follow shortly.

@matmair
Copy link
Contributor Author

matmair commented Nov 16, 2023

@SchrodingersGat this would be ready for review

@SchrodingersGat
Copy link
Member

Thanks @matmair

@SchrodingersGat SchrodingersGat merged commit d30ff8a into inventree:master Nov 19, 2023
24 checks passed
@matmair matmair deleted the bump-weasy branch November 19, 2023 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency Relates to a project dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Error in display of SVG when creating reports - Weasyprint will need to be upgraded
3 participants