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

Better handling of syncing deleted customers #357

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@spr0cketeer
Contributor

spr0cketeer commented Sep 5, 2017

Credit to @tasn for work done in PR #325 - this PR adds tests

Fixes #268

What's this PR do?

Modifies deleted customer webhook to purge customer locally and not attempt to delete customer again on Stripe.

Modifies customer syncing actions and management command to handle situation where customer is deleted remotely but not locally.

Any background context you want to provide?

What ticket or issue # does this fix?

Closes #268

Definition of Done (check if considered and/or addressed):

  • Are all backwards incompatible changes documented in this PR?
  • Have all new dependencies been documented in this PR?
  • Has the appropriate documentation been updated (if applicable)?
  • [x ] Have you written tests to prove this change works (if applicable)?
Better handling of syncing deleted customers
Credit to @tasn for work done in PR #325 - this PR adds tests

Fixes #268
@tasn

This comment has been minimized.

Show comment
Hide comment
@tasn

tasn Sep 5, 2017

Hey,

Thanks for getting to it. Would be better if you kept my commits making the changes (just cherry-pick them) and add your commits adding the tests.

Thanks,
Tom.

Edit: I initially commented on the commit, now I see it shows here in the PR. To maintainers/author: please first include my commit making the actual change, and on top of that include the commit adding the tests so attribution is properly maintained.

Thanks.

tasn commented on f460b62 Sep 5, 2017

Hey,

Thanks for getting to it. Would be better if you kept my commits making the changes (just cherry-pick them) and add your commits adding the tests.

Thanks,
Tom.

Edit: I initially commented on the commit, now I see it shows here in the PR. To maintainers/author: please first include my commit making the actual change, and on top of that include the commit adding the tests so attribution is properly maintained.

Thanks.

@spr0cketeer

This comment has been minimized.

Show comment
Hide comment
@spr0cketeer

spr0cketeer Sep 5, 2017

Contributor

hey @tasn,

New to git and wasn't sure how to proceed so asked the project owners in slack who said a resubmit and acknowledge your work in the PR would be OK.

Contributor

spr0cketeer commented Sep 5, 2017

hey @tasn,

New to git and wasn't sure how to proceed so asked the project owners in slack who said a resubmit and acknowledge your work in the PR would be OK.

@tasn

This comment has been minimized.

Show comment
Hide comment
@tasn

tasn Sep 5, 2017

Contributor

Thanks for your reply, but it's not for them to decide, but rather the author (in this case me).

If you give me access to your forked repo, I can fix the PR for you.
Alternatively, the easiest way would be to amend your commit to remove my fixes, and have this PR only include the tests, and the maintainers would have to merge mine first.

Contributor

tasn commented Sep 5, 2017

Thanks for your reply, but it's not for them to decide, but rather the author (in this case me).

If you give me access to your forked repo, I can fix the PR for you.
Alternatively, the easiest way would be to amend your commit to remove my fixes, and have this PR only include the tests, and the maintainers would have to merge mine first.

@tasn

This comment has been minimized.

Show comment
Hide comment
@tasn

tasn Sep 5, 2017

Contributor

I can see you submitted a new PR (#358). Please close this one.

Just to let you know, you can update a PR by (force) pushing to the PR branch, no need to start a new one.

Contributor

tasn commented Sep 5, 2017

I can see you submitted a new PR (#358). Please close this one.

Just to let you know, you can update a PR by (force) pushing to the PR branch, no need to start a new one.

@spr0cketeer spr0cketeer closed this Sep 5, 2017

@paltman paltman added this to the Samwise milestone Oct 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment