Skip to content
This repository was archived by the owner on Dec 11, 2019. It is now read-only.

Updated to 0.13.0 and payment doesn't process #5974

Closed
bitcoinuser opened this issue Dec 1, 2016 · 11 comments
Closed

Updated to 0.13.0 and payment doesn't process #5974

bitcoinuser opened this issue Dec 1, 2016 · 11 comments

Comments

@bitcoinuser
Copy link

Hi,

I updated to Brave 0.13.0 some hours before Brave process the payment.
Now look what happens: (your next payment contribuition is 20 hours ago)

captura de tela de 2016-12-01 19-11-34

captura de tela de 2016-12-01 19-11-42

@bitcoinuser
Copy link
Author

bitcoinuser commented Dec 1, 2016

I don't know if the payment was processed. And the PDF file was not generated too.

@willy-b
Copy link
Contributor

willy-b commented Dec 1, 2016

Uh oh, I see spaces in the PDF filenames in your screenshot too. Looking.

@willy-b
Copy link
Contributor

willy-b commented Dec 1, 2016

OK, I can reproduce this on Linux with the packaged brave_0.13.0_amd64.deb , but could not reproduce from latest master in development mode.

@bitcoinuser
Copy link
Author

What problem exatly did you reproduce?

@willy-b
Copy link
Contributor

willy-b commented Dec 1, 2016

Not sure about payment failing to go through, but I can reproduce the Payment Receipt PDF generation error. Locally it comes from there being spaces in the PDF filename, which is the case in your screenshot too.

See below:
-- prepackaged build 0.13.0, spaces in filenames:
brave_prepackaged_build_pdf_filenames

-- vs master, no spaces in filenames:
brave_master_pdf_filenames

The code around the filename generation should not be locale sensitive (fixing this), but it happens here because of different behavior of Date.toLocaleDateString (even when locale is forced to be same) in the prepackaged vs. master (Electron difference?), see below.

-- prepackaged build, Date.toLocaleDateString behavior in console:
brave_js_date_behavior_prepackaged_build
(note that specifying options, e.g. "month" as numeric makes no difference here)

-- vs. master, Date.toLocaleDateString behavior in console:
brave_js_date_behavior_master
(note that specifying options, e.g. changing "month" to numeric changes output here)

@willy-b
Copy link
Contributor

willy-b commented Dec 1, 2016

@bbondy , do you know any reason behavior of Date.toLocaleDateString might be different between the prepackaged build and running it locally from source? (even when locale is specified explicitly)

@bsclifton
Copy link
Member

bsclifton commented Dec 1, 2016

@willy-b we are getting ready for a Chromium 54 upgrade; I wonder if you have a newer version locally? you can check about:brave and look at the Muon version for both)

That aside, this could probably be hardened in a way that is much simpler 😄 Instead of using toLocaleDateString() (for the file name). I would recommend to hardcode a format like Brave_Payments_YYYY-MM-DD.pdf

You could try to go the route of replacing spaces with underscore or trimming whitespace on beginning/end of string... but some folks will have their locale setup to use / as the delimiter, which is not valid for Windows. Some locales use a kanji for the day/month/year, depending on the format (which may be a problem saving also)

I think making the filename respect the locale is a nice enhancement (which we could create another issue for), but for now, we go with something that works 😄

@bsclifton bsclifton added this to the 0.13.0 milestone Dec 2, 2016
@posix4e
Copy link
Contributor

posix4e commented Dec 2, 2016

I believe 0.13.0 is the one based off of chromium 54. We need to rebase it badly

@willy-b
Copy link
Contributor

willy-b commented Dec 2, 2016

Sounds good @bsclifton. I changed to the format you suggested, basing my changes on the 0.13.0 commit, which is in conflict with master. The change has been pushed to a branch (see https://github.com/willy-b/browser-laptop/commit/e74670f8f5c1affc2e16383fba40b9004369f858).

But what branch should I open a PR against to get it into 0.13.0?
(I'm guessing either dev-channel or chromium54)
This is important because the feature will be broken for everyone, regardless of locale, due to the change in Date#toLocaleDateString behavior that has popped up in the 0.13.0 branch.

Thanks

@bsclifton
Copy link
Member

@willy-b you can rebase it against master and PR it against master too

The chromium54 branch will need to be rebased against master also (but that'll happen before we build a next release)

@luixxiul
Copy link
Contributor

Would anyone please add QA steps for this issue? 0.13.0 will be the largest release ever and the QA team needs clear and simple steps! Please see #6668 for more info. Thanks in advance 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants