Browser-laptop: Replace Payment History CSV export with Contribution Statement PDFs

[willy-b]

Follow-up to #3477 where we added CSV export to Payment History

This issue tracks replacing the CSV export with a Contribution Statement PDF per @bradleyrichter 's gorgeous original mockup:

[mrose17]

@willy-b - how is this looking? thanks!

[bradleyrichter]

@mrose17 @willy-b I want to get double-duty with this effort on the publisher side as well. These will be great for pubs accounting departments who often need to file docs. Sometimes even printed.

[willy-b]

@mrose17, I'll be pushing some more changes late tonight, thanks

[willy-b]

Latest version in PR #4771:

Note: Time Spent values are not available at the transaction level, so the final financial contributions are shown. (This is probably what we want anyway given that this is a receipt and the Contribution Amounts are only proportional to the Time Spent on average (due to randomization) -- my two cents).

Now, working on pagination of PDF (if enough publishers are present) with different note box text per page as in mockup. Thanks!

[bradleyrichter]

@willy-b Looking awesome! I'm excited to get this in. Just in time for 1.0.

[willy-b]

Sweet. Just pushed pagination :-).

[mrose17]

@willy-b - congrats!

[bsclifton]

I meant to close the original PR :joy:
Re-opening and this'll auto-close when the other is accepted :smile:

[bsclifton]

Merged! Great job, @willy-b! :smile:

[srirambv]

Unable to open PDF on Windows 10 x64 @willy-b @bsclifton

[willy-b]

Thanks @srirambv, wonder what the regression is. Looking into it in VM now.

[bsclifton]

@srirambv is this related to https://github.com/brave/browser-laptop/issues/6330 ?

[willy-b]

@bsclifton, I don't think this is related to #6330 (based on code and screenshot from @srirambv).

6330 should not affect this contribution statement in master branch.

Changes in #6039 which have the Contribution PDF open in the browser after generation make this feature depend on #2714 which may be fixed by #6330.

[willy-b]

To be clearer, the Contribution Statement PDF feature should only depend on PDF.js (#6330) after (or, if) commit https://github.com/willy-b/browser-laptop/commit/fe0ee28f596a0d695529155852d0ec8375acb4e0 is merged

[willy-b]

It looks like ledger-state.json is being updated by you manually there, @srirambv, have you tried generating a transaction history with
npm run add-simulated-payment-history

then checking the resulting PDF?

[willy-b]

also, I get this error even on Linux after commit https://github.com/brave/browser-laptop/commit/bc33beca42e584552ae2ad21a6041980c965094a :

[luixxiul]

@willy-b I have noticed ~ a week ago that the save dialog of the PDF was displayed instead of opening the file in the tab with PDF.js

[willy-b]

That's right, it has never opened in PDF.js (always saved locally via prompt) . To open in PDF.js was requested by @mrose17 in #6039 and will come out with that (in commit https://github.com/willy-b/browser-laptop/commit/fe0ee28f596a0d695529155852d0ec8375acb4e0 _update:_ https://github.com/willy-b/browser-laptop/commit/2c4afcf5e3e31d383f9cc9c53a7baf1df1029392).

[willy-b]

Any word on commit https://github.com/brave/browser-laptop/commit/bc33beca42e584552ae2ad21a6041980c965094a which causes this error on all OS or whether this reproduces using transactions generated with npm run add-simulated-payment-history?

[bbondy]

I commented in the commit but chrome.ipc should be undefined in about page sand chrome.ipcRenderer should be available so I'm not sure why that would break anything.

For example, you will get lots of results if you do:
git grep chrome.ipcRenderer\.

But if you do:
git grep chrome.ipc\.
You only get this one result from the about page that was updated (0 results now)

[willy-b]

An aside:
This is a regression -- and the original feature was released and passed QA, right?
Can we open a new bug?
Sorry this is just hard for me to track. If this is the way it is done here, np, of course, it just seems strange to reopen the original bug instead of filing new one when this was released and passed QA once.

EDIT: Nevermind, I'm in the wrong here. I see this was targeted for 0.13.0 from get-go (despite merge 23d ago) so of course this is getting reopened. My bad.

[willy-b]

@bbondy did anything change with ipc vs. ipcRenderer with the recent Chromium changes / Electron upgrade?

[bbondy]

ya I think it should be another issue but let me try it out here as next step, it might just be a testing error.

[willy-b]

Ah I see, you have actually fixed @srirambv's problem, @bbondy.
With the Chromium changes all the chrome.ipc references were swapped with chrome.ipcRenderer , and so this showed up for all operating systems when Chromium54 changes were merged in.

Original commit for ipcRenderer swap as part of Chromium54:

$ git log --stat -p dbc079 -- js/about/
commit dbc07962d92d5bf73d5b835e0f5fe01830ff1d67
Author: bridiver <[email protected]>
Date:   Tue Nov 8 11:36:20 2016 -0700

    temporary and wip changes for chromium54
---
 js/about/aboutActions.js     | 2 +-
 js/about/adblock.js          | 2 +-
 js/about/autofill.js         | 2 +-
 js/about/bookmarks.js        | 2 +-
 js/about/certerror.js        | 2 +-
 js/about/downloads.js        | 2 +-
 js/about/entry.js            | 2 +-
 js/about/extensions.js       | 2 +-
 js/about/flashPlaceholder.js | 2 +-
 js/about/history.js          | 2 +-
 js/about/passwords.js        | 2 +-
 js/about/preferences.js      | 2 +-
 12 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/js/about/aboutActions.js b/js/about/aboutActions.js
index 3dda621..4a5e6ba 100644
--- a/js/about/aboutActions.js
+++ b/js/about/aboutActions.js
: 
@@ -6,7 +6,7 @@ const messages = require('../constants/messages')
 const serializer = require('../dispatcher/serializer')
 const windowConstants = require('../constants/windowConstants')
 const appConstants = require('../constants/appConstants')
-const ipc = window.chrome.ipc
+const ipc = window.chrome.ipcRenderer

 const aboutActions = {
   /**
diff --git a/js/about/adblock.js b/js/about/adblock.js
index 5e9e9ba..5217138 100644
--- a/js/about/adblock.js
+++ b/js/about/adblock.js
@@ -12,7 +12,7 @@ const aboutActions = require('./aboutActions')
 const ImmutableComponent = require('../components/immutableComponent')
 const SwitchControl = require('../components/switchControl')

-const ipc = window.chrome.ipc
+const ipc = window.chrome.ipcRenderer
[... leaving out rest since ipc changes are identical  ...]

I tested @bbondy's change without the rebuilt Chromium so it appeared to break it, when actually his change should _fix_ the problem observed by @srirambv.

[willy-b]

On latest master with Chromium updated this has a new problem for me though (on Linux too) crashing because the webContents.executeJavaScript method doesn't seem to be available anymore (call in app/pdf.js):

TypeError: wv.executeJavaScript is not a function
    at WebContents.afterLoaded (/home/willy/projs/git/3rd/brave/browser-laptop/app/pdf.js:65:8)
    at emitOne (events.js:101:20)
    at WebContents.emit (events.js:188:7)
process exited with code 7

Looking at changes to webContents.executeJavaScript which came in with Chromium54, e.g.
https://github.com/brave/muon/commit/7a183f20aad1e4668c3b981454c0a35edf577cb3#diff-7e7383250d1f94b76d2fa18a30a3220fR13 to try to understand.

[bbondy]

Ya it was removed and the calls that used it seems to have been removed, there's a similar issue here:
https://github.com/brave/browser-laptop/issues/6433

Pls watch that issue to see what happens there.

[bbondy]

looks like you can do something like this:
webview.executeScriptInTab(config.braveExtensionId, 'code-to-execute-here)

[bbondy]

you can use the webcontents

[bbondy]

I think it would be something like this:
const config = require('../js/constants/config')
wv.executeScriptInTab(config.braveExtensionId, removeCharEncodingArtifactJS, {}, whenReadyToGeneratePDF)

[bbondy]

Looks like there's some other problem with the save as dialog, I'll take a look

[willy-b]

Thanks much for the help!

If Electron/Muon changes are required I will be extra slow as per https://github.com/brave/muon/issues/132 forcing migration to the new browser-laptop-bootstrap, which I admit I'm still trying to get building (init script ran 4.5hrs before failing this AM).

[bsclifton]

@willy-b let me know if you have any specific questions! I've put together some docs which I hope help :smile: Also, a quick heads up: Windows currently has an issue with international / locale which causes a crash on startup. The current "solution" (until we fix ICU) is to rebase the brave/muon code and remove both commits which had ICU related fixes. You can run something like:

cd .\src\electron\
git pull
#should be on master all latest changes
git rebase -i origin/master HEAD~50

then just use d to drop the two commits which have ICU. At that point, you can rebuild and it should work just fine on Windows

Awesome job with the PDF btw- was bummed to see this get broken. @bbondy looks like the remaining errors are with printToPDF?

[willy-b]

thanks @bsclifton, especially for docs.

fwiw, I'm on Linux, not Windows and build is broken for me on this AM's master in browser-laptop and browser-laptop-bootstrap (& brave/muon, formerly electron, no longer has a standalone build process, per https://github.com/brave/muon/issues/132).

[willy-b]

happy to troubleshoot, don't slow yourselves down or anything, just fyi. I'll update later today (realistically this evening) when I hopefully have those two projects building again so I can work on this issue.

thanks!

[willy-b]

OK, so in browser-laptop on master I'm currently not seeing a printToPDF issue, I'm seeing a problem with webview.setTabIndex not being defined. It is invoked in frame.js during the 'on-attach' event for the new tab that opens to render js/about/contributionStatement.js .

continuing to investigate.

[cezaraugusto]

@willy-b trouble with setTabIndex was also reported here: #6420. TL;DR you'd better to clone browser-laptop-bootstrap and build from there for now.

If you're blocked due to setTabIndex errors (and building from bootstrap can take a while), you can revert (locally) the below commits:

• git revert 901d28cf5d752d707bd6ab2cb4351547749d6096
• git revert 3004b9e80e4bd5ba9fcabea53e627e3144e26dda

[willy-b]

thanks for info @cezaraugusto. knowing which commits to revert to unblock is a big one.
still working on browser-laptop-bootstrap in background.

FYI, the setTabIndex issue on new tab open is the same as #6442 which was opened and closed this AM. will be fixed with that.

[willy-b]

Okay, after doing workaround for setTabIndex issue I see the printToPDF issue.

Since for #6039 / #6385 we are switching from doing a popup "Save as" to opening the generated PDF in PDF.js, I am going to experiment with just switching that out now.

[bbondy]

@willy-b I'm going to fix printToPDF in #6450 so please feel free to close this if it is done otherwise.

[willy-b]

to the best of my knowledge this is fine but for #6450 & #6442 (can't tell until those are fixed though).

thanks for taking #6450, I was working on that right now but I'm sure you'll get it faster.

(opening the payment history pdf in PDF.js will continue in PR #6385 for MTR's issue #6039)

[willy-b]

actually let's leave it open until #6450 and #6442 are in so I or whoever can test this before it goes out.

[willy-b]

if @bbondy's #6450 fix for webview.printToPDF API in brave/muon can't make it into browser-laptop by time next release with Chromium54 goes out then we should probably revert this entire feature because it will be broken.

[bbondy]

It'l make it in ;)

[willy-b]

cool, I just don't want this feature (PDF stmt) to make it in if it can't be tested / is broken :-). thanks.

[bbondy]

This was committed so next preview should have it in (even maybe some platforms current preview), so closing because I think there's nothing remaining here.

[willy-b]

let's keep it open until those last Muon changes (https://github.com/brave/muon/commit/465539ead14fb035185ff6aff6492eff14420348) actually hit browser-laptop and can be tested -- there might be a regression or who knows, right?

[bbondy]

Sorry I should have explained the process better that we follow for QA. When the coding is complete they should be marked completed and QA works off of bugs that are in the completed state to verify them.

[bsclifton]

@willy-b what @bbondy said :smile: You can watch this issue and see the QA labels that get attached. If everything works out fine, you'll see it get tagged like so:

[luixxiul]

@willy-b would you please specify QA steps or the commit which has them in its commit message?

[luixxiul]

Also please indicate which PR has actually closed this issue (because there are several PRs which were referenced with this issue it is hard to find which one it is), thanks.

[luixxiul]

Lastly, please add QA/no qa required to issues and PRs which do not require QA.

Thanks!

[willy-b]

@luixxiul the changes which fixed this are @bbondy's from #6450:

In Preview7, already tested:

• https://github.com/brave/muon/commit/9ea426b623d79450272736789d4e651669ad5230 in brave/muon fixed webview.printToPDF everywhere but Windows (this was in Preview7, I can confirm this works)

Not in Preview7, not yet tested afaik:

• https://github.com/brave/muon/commit/465539ead14fb035185ff6aff6492eff14420348 supposed to fix webview.printToPDF on Windows
  NOTE: overlaps heavily with previous changes, so should test on all platforms after this comes into brave/browser-laptop