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

OAuth support #242

Closed
wants to merge 12 commits into from
Closed

Conversation

YouneselBarnoussi
Copy link
Contributor

@YouneselBarnoussi YouneselBarnoussi commented Apr 28, 2024

Now, payment processing with OAuth isn't supported in the package. This could be handy for SaaS apps where organizations manage subscriptions and payments for users. It's a significant change, but I made sure it's backward compatible, so it still functions without OAuth. Hopefully, this can be considered for the next major release.

This PR does depend on this PR in mollie-api-php

Edit: I will fix the tests anytime soon and add other tests but I opened it for now so it can already be reviewed/looked at.

@Naoray
Copy link
Collaborator

Naoray commented May 13, 2024

Hi @YouneselBarnoussi,

thank you for your time investment to write this big chunk of code 😄!

Can you please describe the exact use case you need this for? You might also want to consider using Organization access tokens (s. https://docs.mollie.com/overview/authentication for an explanation about the differences).

Apart from that, this is a big chunk of code - at least when considering how many people use it and how many applications this might break if there is a bug somewhere. I will take your PR into consideration and talk to the team about it, but it will take time till we release a new major version.

@YouneselBarnoussi
Copy link
Contributor Author

Hey @Naoray,

I will expand a bit on the initial explanation. Let's say I am the owner of an app that allows sport clubs to manage their user data and subscriptions. The application is multi tenant, which means they share the same Laravel and database instance. Two independent clubs register for my app. The app has an OAuth intergration with Mollie using Socialite which makes it easy for the club owners to connect their Mollie account. Both owners register a user and want to start a subscription for that user.

As a database/model structure it would look like this. User -> ClubUser (is the billable model) -> Club (provides OAuth information).

Now there needs to be a way to know for which Mollie account the payment should be created. With the current implementation of the package it would create all payments on my account (the app owner). This PR gives the app owner the opportunity to define for which Mollie account the payment should be created.

This is done by adding the ProvidesOauthInformation interface to the ClubUser model/pivot.

class ClubUser extends Model implements ProvidesOauthInformation;
{
    /**
     * Get the OAuth token.
     */
    public function getOauthToken(): string
    {
        return $club->mollie_access_token;
    }

    /**
     * Get the Mollie Profile ID.
     */
    public function getMollieProfile(): string
    {
        return $club->mollie_profile_id;
    }

    /**
     * Get whether the Mollie API is in test mode.
     */
    public function isMollieTestmode(): bool
    {
        return ! app()->environment('production');
    }
}

Let me know if there are any other questions/concerns.

@Naoray
Copy link
Collaborator

Naoray commented May 15, 2024

Hi @YouneselBarnoussi,

thanks for the clarification of your use case! As I mentioned before, I ll need to consult with the rest of the team and let it sink in :).

@sandervanhooft
Copy link
Collaborator

Hi @YouneselBarnoussi ,

I can see a lot of effort into this PR, which is why we've seriously weighed the merits of merging it.

After careful deliberation we've come to the conclusion however that this PR's use case is too far beyond the intent of this package.

@YouneselBarnoussi
Copy link
Contributor Author

Hey @sandervanhooft

No worries at all, I will keep my fork running separately in that case!

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