-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
PhantomJS clipRect not clipping appropriately #52
Comments
Still haven't quite worked this out. I have made a number of improvements to the code, but there are still issues. See https://github.com/ongoworks/reaction-core/commit/57820b15866b3cc4c1d90558594f6fbb42b8bb29
|
See http://stackoverflow.com/questions/14748840/meteor-router-phantomjs Kind of similar to the issue with |
I wrote this method a few months ago, relying on fibers instead of _wrapAsync, which I couldn't quite get working right. (I think I was trying to callback with callback, and wasn't understanding how to use). Always knew this needing rewriting, this code reads much better now. Also wrote this before adding CollectionCFS, or Blaze so it was time for a complete redo. On clipRect - that is similar to the behavior I was getting - clipRect had the appropriate dimension to clip, but the pdf wasn't generating. I've been rewriting the sub-pub methods, moving them into iron-router and haven't tested every scenario - I was subscribing to shop in subscriptions.coffee - I suspect that could the cause of pub/sub timing issues. I was/am trying to narrow down the subscription records in general to only publish exactly what is needed. In particular I did this on the order / completed route. |
The Meteor docs don't expressly say that a publish function must return a cursor or call In any case, I will go through the publish functions and ensure that they all are written correctly. |
OK, now committed fixes to the publish functions and revised waitFor/setInterval code, and that piece is working well. The remaining problems are (1) the clipRect still isn't being respected on render and (2) I don't see any of the data in the PDF in my tests (but maybe I'm doing something wrong). |
Also, I'm not sure about the security of it all. Sometimes it seems to know which user I am within phantomjs and other times it doesn't and the "Unauthorized" page is saved to the PDF instead. I would think you would have to somehow fake a login within phantomjs in order to see the proper page and proper data. |
re: security, you should have to be logged in as an admin for an order other than one you created to be accessed - so that's an interesting point, maybe sometimes it's getting a different session? |
I was logged in as an admin and also placed the order in question with that same admin user account, so if there were a case where it should have worked consistently, that would be it. I'm a bit surprised that it would ever work without "logging in" to the phantom page. I half suspect that since I'm testing in Chrome and running the server code locally, they are somehow sharing localStorage, which is where Meteor tracks the user/session. |
I tested under the same scenario and got the "unauthorized" but when loading the actual url (in chrome) repeatedly - never was able to get "unauthorized". I'm always seeing clipRect is set to { height: 0, left: 0, top: 0, width: 0 }. If I change the pub userOrders clipRect gets values, so I think maybe we should use a different route than completed (ie / orders/) that uses the admin / find all. However, back to the same problem - clipRect even with values, seems to not be clipping.... |
I committed a fix to the login issue, but it requires that the client code calling the Meteor.call("createPDF", "/completed/co5goJqebo6tAqLgw", "co5goJqebo6tAqLgw", Accounts._storedLoginToken(), function () { console.log(arguments); }) As for clipRect, I believe it is this issue: ariya/phantomjs#10465 I think we will have to do the PNG to PDF workaround? |
Argh. Yes, that looks like the issue. |
Or maybe the printable routes could support a query string param which, when present, omits the layout? You'd then just have to add the query param to the |
that's even better, one step further - we could have a print layout that is used instead of the default layout. Would allow for customization, logo's styling etc. |
@aldeed Perhaps we should rip this out now, and use Blaze.toHTMLWithData + https://github.com/MrRio/jsPDF to create PDFs. |
Actually that would be an awesome approach if it works because we could offload all PDF generation computing power to the client machines. |
@aaronjudd, are you actively working on this? I might need this for another app, so I can try it out and create a pkg for it if you're not. |
it's on my shortlist, but not actively working on it - wanted to get it in the v0.2.2 release, so if you can work on it that would be awesome |
The approach using jsPDF and Blaze.toHTMLWithData seems to work. Tomorrow I'll take what I've done and put it into a package. Regarding how to use it in RC: I'm thinking in most cases we'll list a "document" even though no such document has ever been generated, and then when they click the download button for it, we'll make and save the PDF just in time? (It's pretty much instant.) This has the benefit of the documents always being up to date, but on the flip side, are there cases where a real PDF should be generated and stored permanently for legal reasons? If so, we can still generate on the client (one time) and then upload the result using CFS. (Client-side computing power is cheaper.) Subsequent downloads would be real downloads from CFS then. |
I don't think we need to store the pdf, although you are right there might be some legal reason in the future - however I think for now let's go with just generating as needed. In most cases, the order records should be a "up to date, but historical" document anyways, and once we add revision control, we'll be including orders as well. |
Good point. The only other consideration will be offline tasks, like e-mailing. If there's anything that you'll want to attach to e-mails as a PDF, we'd need to sneakily generate it on the client as part of whatever action initiates it, then upload to the server for later attachment. Or... wrap in phantomjs. :) |
hopefully we can just send html emails, and avoid those ugly pdfs server side - I really can't think of a reasonable reason for sending PDFs or storing them in this century |
Closed with the implementation of jsPDF in v0.2.2, although that package needs more testing as well as a new issue. |
…update-feb-13-2019 Daily update feb 13 2019
…aws-sdk-2.1030.0 chore(deps): Bump aws-sdk from 2.796.0 to 2.1030.0
Debug phantom.create syntax on /packages/reaction-commerce/server/methods/orders/methods.coffee line 59, createPDF method:
Should only clip area within a class "print-area"
The text was updated successfully, but these errors were encountered: