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

Updated Apps, payment packages #1592

Merged
merged 26 commits into from
Nov 28, 2016
Merged

Updated Apps, payment packages #1592

merged 26 commits into from
Nov 28, 2016

Conversation

aaronjudd
Copy link
Contributor

@aaronjudd aaronjudd commented Nov 27, 2016

  • single card payment layout
  • updated / refactored Reaction.Apps
  • normalization of client Packages moved to publication
  • PayPal and PayFlow split out
  • removes default payment method and collapse handling in checkout
  • commented dashboard default method settings. TBD.
  • resolves Group payment methods into single app card #1408
  • adds object-curly-spacing lint rule
  • adds significant i18n to payments, makes local
  • adds translations to month helpers

aaronjudd added 19 commits November 10, 2016 22:15
Updated package registry handling, updates for payments.
Resolve `websocket because it does not appear in the connect-src
directive of the Content Security Policy` error in safari when using
—production build.
- removes name from some payment method registry entries
- use name if separate settings and permissions are needed
- applies permission checks in package publication for filtering
enabled views
- uses package name as default settings key,  used when no name
- Cleanup handling in taxes, discounts, catalog package handling
- Fix error where registry update was overwriting existing settings
- use mergeDeep for registry updates
- remove excessive panels
- add i18n
- generic alerts
- convert panel-title to alert info
- show alert message only pre-setup
momentjs:moment  upgraded from 2.16.0 to 2.17.0
- Process multiple matching entries in registry
- adds i18n for PayPal client views
- splits PayPal into two payment methods for settings/checkout
- todo add migration for express_enabled to express.enabled
- todo add migration for payflow_enabled to payflow.enabled
- add browser policy akamai.mathtag.com
- apply standard button style + i18n to all payment settings
- single card payment layout
- removes default payment method and collapse handling in checkout
- commented in dashboard payment settings. TBD.
- resolves #1408
// Meteor.call("discounts/setRate", cartToCalc._id, discountRate);
if (cartToCalc.discountMethods) {
for (discountMethods of cartToCalc.discountMethods) {
// get discount based on id in cartMethods.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this just a placeholder for a forthcoming implementation of the discount calculation?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just wanted to double-check that this intentionally does nothing here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.. it's no harm, no foul at this point...

import { Reaction, i18next } from "/client/api";
import { Packages } from "/lib/collections";

import {Meteor} from "meteor/meteor";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we keep the spaces around imports consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently a beautifier rule that wasn't working before.

@aaronjudd
Copy link
Contributor Author

No, this should go ahead and be merged -> it does have the placeholders for rates, and codes, but it's not really related functionality at this point (although had to be done to add codes in the checkout the way I want)

import _ from "lodash";
import {Reaction} from "/client/api";
import {Packages} from "/lib/collections";
import {Template} from "meteor/templating";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bracket spacing, import order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have a rule to enforce this? "space_in_paren": true in the .jsbeautifyrc will add the spaces on save, but also adds in a bunch of other places. Eslint doesn't see anything wrong here.. which would leave me back to disabling .jsbeautify.. (nobody else using this I guess?).

@@ -9,6 +9,7 @@ import { Reaction } from "/client/api";
import { Shops, Translations } from "/lib/collections";
import * as Schemas from "/lib/collections/schemas";
import i18next, { packageNamespaces, getLabelsFor, getMessagesFor, i18nextDep } from "./main";
import {mergeDeep} from "/lib/api";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bracket spacing

Copy link
Contributor

@jshimko jshimko Nov 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we need to add the space-in-brackets rule (and make arrays an exception).

http://eslint.org/docs/rules/space-in-brackets

Probably something like...

"space-in-brackets": [2, "always", {
  "arraysInArrays": false,
  "arraysInObjects": false
}]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule was removed in ESLint v1.0 and replaced by the object-curly-spacing and array-bracket-spacing rules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alrighty...

"object-curly-spacing": [2, "always"],

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  "sort-imports": ["error", {
        "ignoreCase": false,
        "ignoreMemberSort": false,
        "memberSyntaxSortOrder": ["none", "all", "multiple", "single"]
    }],
    "object-curly-spacing": ["error", "always", { "objectsInObjects": true }],

However, that's going to change 90 files, as --fix rightfully adds spacing to objects all over the place. I'm actually cool with that, but it's new...

Sorts rule isn't quite advanced enough to enforce our rules (I think this basically is just alphabetical).

@brent-hoover
Copy link
Collaborator

Checking out with the Example payment method I see this error in the console. Order does complete however.


meteor.js?hash=e3f53db…:930 Exception in template helper: TypeError: moment is not a function
    at Object.<anonymous> (http://localhost:3000/app/app.js?hash=65db4f0c5205ad7c0f0dd9cb714deba07c0401e4:50923:12)
    at http://localhost:3000/packages/blaze.js?hash=10a350f1c0d55cf44d3c99989b04ef2e7e0d23f6:3035:16
    at http://localhost:3000/packages/blaze.js?hash=10a350f1c0d55cf44d3c99989b04ef2e7e0d23f6:1697:16
    at http://localhost:3000/packages/blaze.js?hash=10a350f1c0d55cf44d3c99989b04ef2e7e0d23f6:3087:66
    at Function.Template._withTemplateInstanceFunc (http://localhost:3000/packages/blaze.js?hash=10a350f1c0d55cf44d3c99989b04ef2e7e0d23f6:3728:12)
    at http://localhost:3000/packages/blaze.js?hash=10a350f1c0d55cf44d3c99989b04ef2e7e0d23f6:3086:27
    at Spacebars.call (http://localhost:3000/packages/spacebars.js?hash=ebf9381e7fc625d41acb0df14995b7614360858a:172:18)
    at Spacebars.mustacheImpl (http://localhost:3000/packages/spacebars.js?hash=ebf9381e7fc625d41acb0df14995b7614360858a:106:25)
    at Object.Spacebars.mustache (http://localhost:3000/packages/spacebars.js?hash=ebf9381e7fc625d41acb0df14995b7614360858a:110:39)
    at Blaze.View._render (http://localhost:3000/app/app.js?hash=65db4f0c5205ad7c0f0dd9cb714deba07c0401e4:25685:24)

@brent-hoover
Copy link
Collaborator

I wonder if we shouldn't throw up a warning in the settings panel if no Payment packages are enabled?

@brent-hoover
Copy link
Collaborator

Seeing this message (not an error) in the console when PayPal PayFlow is enabled:

modules.js?hash=09d32e0…:47071 Deprecation warning: use moment.updateLocale(localeName, config) to change an existing locale. moment.defineLocale(localeName, config) should only be used for creating a new locale See http://momentjs.com/guides/#/warnings/define-locale/ for more info.

@brent-hoover
Copy link
Collaborator

It doesn't look like we do a client-side validation on Expiration date? For Paypal Payflow I got this error when I submitted a card with an invalid expiration.

cart_checkout

@aaronjudd
Copy link
Contributor Author

I did update the way moment is loading, so the first error could be related (try to get to that one reproduced right now). Re: the deprecation warning.. that's because we need more garbage debugging on that page that we can't shut off. oh no wait, it's because I am using defineLocale to add an alias to ZH, which is what it seems to be indicating is the correct purpose, not the deprecated purpose.... I think..

@aaronjudd
Copy link
Contributor Author

re: client side validation - no, there is not any validation to speak of other than autoform stuff. nothing new here. however, I've been talking about having a credit card / payment method component for a while, to replace this input form with a nice modern input validator, aka card.js etc. I was holding myself back, but if you are suggesting it's a must have..

@@ -16,7 +16,9 @@
},
"paymentSettings": {
"authorizenetLabel": "Authorize.net",
"authorizenetSettingsLabel": "Authorize.net"
"authorizenetSettingsLabel": "Authorize.net",
"authorizenetCredentials": "Don't have a AuthNet API Login ID and Transaction Key?",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be "an Authnet"?

@brent-hoover
Copy link
Collaborator

Approved - Except for the couple of minor things I think this is a nice step forward, especially on the consumer-side where I thought that part was probably confusing.

@brent-hoover
Copy link
Collaborator

It seems like Paypal Express is enabled by default?

@brent-hoover
Copy link
Collaborator

brent-hoover commented Nov 28, 2016

This checkbox looks a little odd (Paypal Express)

@brent-hoover
Copy link
Collaborator

If I have no payment methods specified, in FF, instead of seeing the "No Payment" message I see all payment methods

cart_checkout

@aaronjudd
Copy link
Contributor Author

aaronjudd commented Nov 28, 2016

the way it's setup now, you'll see all the payment methods if you are logged in as admin in checkout, the view restriction is permission based now (or at least partially) (see the transform in Packages publication). I'd like to see us narrow this down with an implementation of "audience" permissions checks. The consumer/client should only be seeing what's enabled.

@aaronjudd
Copy link
Contributor Author

re: defaults, I set the initial to have "PayPal express" and "Example" enabled.

aaronjudd added 4 commits November 27, 2016 19:31
- could have just used if in template, or alternate display
- but this is clean and simple for now.
Fix error loading moment for template helpers.
- remove Authnet/dashboard - unused legacy settings?
@aaronjudd
Copy link
Contributor Author

@zenweasel All issues you reported should now be addressed. Ready to merge.

@aaronjudd aaronjudd merged commit a1bae0b into release-0.18.0 Nov 28, 2016
@aaronjudd aaronjudd deleted the package-settings branch November 28, 2016 15:48
@aaronjudd aaronjudd removed the review label Nov 28, 2016
@spencern spencern mentioned this pull request Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants