Skip to content

Conversation

@Myztiq
Copy link
Contributor

@Myztiq Myztiq commented Aug 22, 2016

Actually hooked up with real API routes, got saving and updating of CC working.

My apologies, this has kind of evolved and now encompasses everything needed to get payments pushed to production. I'll freeze this branch so it can be reviewed and make any new changes in a v4 branch.

@Myztiq Myztiq added the hold label Aug 22, 2016
@runnabot
Copy link

The latest push to PR-1678 is running on SAN-4781-payments-integration-v3-runnable-angular

@runnabot
Copy link

runnabot commented Aug 22, 2016

The latest push to PR-1678 is running on SAN-4781-payments-integration-v3-angular-unit

@Myztiq Myztiq added pending-tests and removed hold labels Aug 22, 2016

$scope.getBillingDate = function (invoice) {
return moment(invoice.period_end * 1000).format('M/D/YYYY');
return moment(invoice.periodEnd).format('M/D/YYYY');
Copy link
Member

@podviaznikov podviaznikov Aug 23, 2016

Choose a reason for hiding this comment

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

if we have now non US customers (and I think we have customers from Ireland) I would suggest to change M/D/YYYY format. For most people outside of US this one can be super confusing.
I would do something like Aug 25, 2016 - that would be unambiguous

@Myztiq Myztiq assigned thejsj and unassigned anandkumarpatel Aug 23, 2016
link: function ($scope) {
$scope.currentOrg = currentOrg;
$scope.hasUpdated = false;
$rootScope.$on('updated-payment-method', function () {
Copy link
Member

Choose a reason for hiding this comment

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

You need to $off this on $scope.destroy.

Copy link
Member

Choose a reason for hiding this comment

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

well, call the unregister function

Copy link
Member

Choose a reason for hiding this comment

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

Also, does this need to listen for this on the rootscope?

@thejsj
Copy link
Member

thejsj commented Aug 23, 2016

Code LGTM. Doing more E2E testing.

@thejsj
Copy link
Member

thejsj commented Aug 23, 2016

Super minor: If I add a payment method to an org that's in an active period already, this shows up. I would necessarily fix this since there should be no state in which an org is in this state, but just to point it out.

screen shot 2016-08-23 at 1 34 46 pm

@thejsj
Copy link
Member

thejsj commented Aug 23, 2016

I think Stripe creates these for test accounts. Not sure if we want to filter them?

screen shot 2016-08-23 at 1 37 01 pm

@Myztiq
Copy link
Contributor Author

Myztiq commented Aug 23, 2016

Ehh, I don't care about people being in weird states seeing odd things. It's not a state that should be possible to be in.

Would you like me to filter those out?

@thejsj
Copy link
Member

thejsj commented Aug 23, 2016

LGTM

@henrymollman
Copy link
Contributor

LGTM

@Nathan219
Copy link
Member

LGTM

@thejsj
Copy link
Member

thejsj commented Aug 24, 2016

I know I LGTM this PR, but can we confirm this is expected behavior (still seems weird to me):

http://g.recordit.co/8MGQKXk6xT.gif

@Myztiq Myztiq merged commit 13e3be0 into master Aug 24, 2016
@Myztiq Myztiq deleted the SAN-4781-payments-integration-v3 branch August 24, 2016 17:25
@Myztiq
Copy link
Contributor Author

Myztiq commented Aug 24, 2016

That is tracked in a bug :)

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.

9 participants