CustomerService suggestion about medusa-auth-cognito HOT 9 CLOSED

Hi, Adrien. Thanks so much for taking a look.

If I define existing methods in the Customer service (like create and update), don't I need to copy of the existing functionality within those methods so that it is not lost when overwritten? EDITL I've seen now that you are calling the parent class functions using super. It is a nice approach, but I think it doesn't provide the necessary level of integration within those underlying functions. For instance, in create, you don't want to save the customer without knowing first if creating the cognito customer and setting the password succeeded. It should all fail or succeed together. I'm not sure saving the customer, getting a cognito failure (maybe the service is down), and then deleting the customer would be sufficient since it it a softdelete. It could work for the delete function. I will go ahead and change that one.

I think ideally the direction I would love to go with this is to have those existing functions like create and update make calls to a new standard service, the same as for search, files, notifications, etc. That way, the interface can be properly defined, and no changes at all will need to be made to the customer or auth services.

from medusa-auth-cognito.

The places where I clearly marked the changed sections are the places that I would envision being standardized calls to the new credentials service.

from medusa-auth-cognito.

When you override a method, you can still call the super one and it will call the one from the parent class. With the changes that I have made, it is just like if you was adding hooks to the customer service :). But I agree and I ve discussed that with oli that it could be nice to have some hooks that call a common service such as what I ve done with the old pr I ve proposed ;)

from medusa-auth-cognito.

For instance, in create, you don't want to save the customer without knowing first if creating the cognito customer and setting the password succeeded.

We are in a transaction, if the cognito service fail, the entire transction will be rollback and user will not be created/updated. This is why I intentionnaly wrapped everything in a single atomicpahse. The super method when using the atomicPhase will reuse the same transactionManager that has been created by the one I have added. Basically, you can have any number of atomicPhase if the transctionManager already exists on the class then it will re use it

from medusa-auth-cognito.

Hmm, I get it. This is very helpful and will definitely address issues of not having to copy over changes in the underlying functions as they are updated.

Still, I think hooks would better address the issue of multiple plugins extending the same underlying service, if I understand correctly how those might conflict. If, for instance, someone has already made some customizations to those methods, there could be a conflict. Probably best to do those via subscribers, but you never know what modifications a user may be running.

from medusa-auth-cognito.

Yeah I agree, I am not sure for the subscriber approach as it will run asynchronously. Meaning that if something fail in the subscriber the customer is not aware of it immediately and you would need a poling/ws mechanism to have the customer wait while the event is being consumed, but again, here you are not sure when the event will be consumed depending on the number of events to process 😂

I think that having a single service that implement a certain interface and is called by the different part of the auth flow is a better approach (as you have mentioned and how I suggested with the old pr you found :) ). That being said, since we are moving to modules, would it make more sense to wait to create the auth module that would encapsulate all this flow in a single place and add the flexibility on top of it?

from medusa-auth-cognito.

Yeah, definitely don't want to use subscribers for this, partly because of the passing around of passwords. I just meant I can only hope others wanting to modify the customer.create and customer.update procedures are using subscribers triggered by those events and not modifying them directly, or there will be conflict.

I'm all for the new service. Let's do it. :) :) :)

I don't fully understand yet what the new module architecture will look like.

from medusa-auth-cognito.

I've been thinking more about this, and I think maybe it's not necessary to create a completely new service. Maybe all that is needed is some slight refactoring to enlarge the existing auth service.

Rather than having the customer service handle small bits of auth-related functionality in create, update, and delete, those 3 functions would instead make calls as needed to newly defined functions in the existing auth service.

In this approach, rather than having a cognito or supabase or whatever service, those functions would instead be handled in auth.ts.

So, I would propose this for a PR to the core: Existing customer service would call auth from inside customer.create, update, and delete. Auth would have a few new functions that do things exactly as they are
now by default, with the password hash in the database.

What are your thoughts?

I think it would be best to have separate auth services for users vs customers (or admin vs store). This way, you could easily define a different primary credentials provider for each via plugins. But that would be for a later time.

Aside from the ideas of splitting auth into 2 separate services, the changes needed to the core would be pretty minor.

I think I will move toward this approach when I work on implementing your code above. The extension of customer will be refactored to call upon super as above, and cognito will be merged into the extension of auth instead of being standalone.

from medusa-auth-cognito.

Changes based on the suggestions in the OP were made in commit 4f2e816.

Thanks again, Adrien.

from medusa-auth-cognito.