Move user and customer updates into a transaction, managed by a module #501

begedin · 2016-11-28T14:29:33Z

What's in this PR?

Adds a UserManager module, which, for now, is placed under lib/code_corps/user_manager.ex, but ought to probably moved into a new namespace, together with DonationGoals manager.

This module, for now, exposes an update function, which receives a user and a set of attributes.

The update function behaves in the following way:

If the update does not change the user's email, then it only updates the user
If the update does change the user email, and there is a platform customer associated with the user, it updates the platform customer as well, both on stripe and locally.

To update the platform customer, I've also implemented StripeService.StripePlatformCustomer.update/2, which handles updating of both the local and the API record.

The scope of the PR was unclear to me, so I implemented the whole process. Based on the issue title alone, the assumption would be we just need to pass in the parameters during customer creation. However, the issue description also mentions keeping it up to date so "we can search by user email".

Questions:

Right now, it seems like the email is the only field that makes sense to update. A platform customer also has a description field, so I guess we could be setting something there as well.
Initially, I was going to perform connect customer updates as well, but we aren't storing anything locally that could conceivably be updated. The email might need to be kept up to date on stripe, but we are not storing it locally.

References

Fixes #489

joshsmith · 2016-11-28T16:58:07Z

We discussed that we're going to need to change this to push out connect customer email changes, but that we won't store the email on our server.

joshsmith · 2016-11-28T18:10:57Z

Marked as needs work but still needs code review, too.

joshsmith · 2016-11-29T03:14:06Z

I am marking as low priority at the moment and removing the needs code review for the time being.

There is no mechanism right now for a user to update their email through Code Corps, and that's the only change going to Stripe.

Once we allow users to change their email – or once we have time to circle back – we can reprioritize this.

Let's not let it get out of date.

We still need the change mentioned above.

joshsmith · 2016-12-03T03:32:51Z

I think we actually need to finish this up. Was wrong.

begedin · 2016-12-05T10:53:59Z

I made some updates to this, so it ought to be ready for review.

When a user updates, if the updates contain an e-mail,

any associated platform customer record will also update
any associated connect customer records will also update

UserService.update(user, attributes) will return one of the following:

{:ok, %User{}, :nothing_to_update, :nothing_to_update}
{:ok, %User{}, %StripePlatformCustomer{}, :nothing_to_update }
{:ok, %User{}, %StripePlatformCustomer{}, [{:ok, %StripeCustomer}, ...]
{:error, changeset}
{:error, :unhandled}

Considering the amount of potential overlap with #508, I would prefer to merge this before touching #508 further.

joshsmith

Some comments.

joshsmith · 2016-12-06T03:04:56Z

lib/code_corps/services/user_service.ex

+      {:error, _failed_operation, _failed_value, _changes_so_far} ->
+        {:error, :unhandled}
+      other ->
+        IO.inspect(other, pretty: true)


This needs removed. Is there an other case that needs handled?

joshsmith · 2016-12-06T03:06:36Z

lib/code_corps/services/user_service.ex

+
+  defp do_update(%Changeset{} = changeset) do
+    with {:ok, user} <- Repo.update(changeset) do
+      {:ok, user, :nothing_to_update, :nothing_to_update}


Why not return nil and nil here?

:nothing_to_update seems overly specific here whereas nil is just as descriptive, IMO.

joshsmith · 2016-12-06T03:10:38Z

web/controllers/user_controller.ex

-    |> Repo.update
-    |> CodeCorps.Analytics.Segment.track(:updated_profile, conn)
+  def handle_update(conn, record, attributes) do
+    with {:ok, user, _platform_customer_updates, _connect_customer_updates} <- UserService.update(record, attributes)


I would just go unnamed on the nil returns here, straight _. See no harm in that and easier to read.

It feels like the UserService.update should probably happen in an asynchronous task rather than sync here.

We can either do it in a transaction, meaning we have to wait for sync, or we ditch the transaction, update the user, respond with that and do the propagation async. I don't think we can do both.

joshsmith · 2016-12-06T03:11:15Z

web/models/stripe_connect_customer.ex

+
+  def update_changeset(struct, params) do
+    struct
+    |> cast(params, [:email])


Why no validate_required here? as there is with platform?

That changeset should go away completely, actually. I added it by mistake, before realising there is nothing to update with the local record.

joshsmith · 2016-12-06T03:11:46Z

test/lib/code_corps/stripe_service/stripe_platform_customer_service_test.exs

+      {:ok, customer} = StripePlatformCustomerService.update(customer, %{email: "mail@mail.com"})
+      assert customer.email == "mail@mail.com"
+
+       # TODO: Figure out testing if stripe API request was made


Is this TODO leftover or intentional?

It's intentional. I'm not sure how to check if a stripe API update was made. Maybe the function should return both the updated local record and the updated stripe record?

I'm starting to think we should avoid "side effects" in our functions as much as we can. I added a response containing updated platform and connect customer records to the user service for basically the same reason.

joshsmith · 2016-12-06T03:12:17Z

lib/code_corps/services/user_service.ex

+  Handles CRUD operations for users.
+
+  When operations happen on `CodeCorps.User`, we need to make sure changes
+  are propagated to related records, ex., `CodeCorps.StripePlatformCustomer` or


Should be and, right?

joshsmith · 2016-12-06T03:12:28Z

lib/code_corps/services/user_service.ex

+  """
+
+  alias CodeCorps.{Repo, StripeConnectCustomer, StripePlatformCustomer, User}
+  alias CodeCorps.StripeService.{StripeConnectCustomerService,StripePlatformCustomerService}


Space after comma.

joshsmith · 2016-12-06T03:20:04Z

@begedin feel free to merge even if there are unresolved questions here, but if so (like the async task), open in another issue and reference this.

…latform and connect customer updates

begedin added this to the Donations milestone Nov 28, 2016

begedin added the needs code review label Nov 28, 2016

joshsmith added the needs work label Nov 28, 2016

joshsmith force-pushed the 489-pass-parameters-to-stripe-customer branch from e4758b6 to 2c3bfca Compare November 29, 2016 03:11

joshsmith added the Priority: Low label Nov 29, 2016

joshsmith removed the needs code review label Nov 29, 2016

joshsmith removed this from the Donations milestone Nov 29, 2016

begedin mentioned this pull request Nov 29, 2016

StripePlatformCustomer updates StripeConnectCustomers on update #475

Closed

2 tasks

joshsmith added Priority: High and removed Priority: Low labels Dec 3, 2016

joshsmith added this to the Improve Donations milestone Dec 3, 2016

joshsmith assigned begedin Dec 5, 2016

begedin force-pushed the 489-pass-parameters-to-stripe-customer branch 2 times, most recently from 3beb43a to f4e361e Compare December 5, 2016 10:39

begedin force-pushed the 489-pass-parameters-to-stripe-customer branch 5 times, most recently from 5a4d168 to 4107099 Compare December 5, 2016 11:07

begedin added needs code review and removed needs work labels Dec 5, 2016

joshsmith reviewed Dec 6, 2016

View reviewed changes

joshsmith added needs submitter response and removed needs code review labels Dec 6, 2016

begedin force-pushed the 489-pass-parameters-to-stripe-customer branch 2 times, most recently from c9d7c6e to 64f568e Compare December 6, 2016 10:35

Move user updates into a transaction, which also manages associated p…

631b556

…latform and connect customer updates

begedin force-pushed the 489-pass-parameters-to-stripe-customer branch from 64f568e to 631b556 Compare December 6, 2016 11:04

begedin removed the needs submitter response label Dec 6, 2016

begedin merged commit 1e66ece into develop Dec 6, 2016

begedin deleted the 489-pass-parameters-to-stripe-customer branch December 6, 2016 11:16

Move user and customer updates into a transaction, managed by a module #501

Move user and customer updates into a transaction, managed by a module #501

Uh oh!

Conversation

begedin commented Nov 28, 2016

What's in this PR?

Questions:

References

Uh oh!

joshsmith commented Nov 28, 2016

Uh oh!

joshsmith commented Nov 28, 2016

Uh oh!

joshsmith commented Nov 29, 2016

Uh oh!

joshsmith commented Dec 3, 2016

Uh oh!

begedin commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshsmith commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

begedin commented Dec 5, 2016 •

edited

Loading

begedin Dec 6, 2016 •

edited

Loading

begedin Dec 6, 2016 •

edited

Loading

joshsmith commented Dec 6, 2016 •

edited

Loading