feat(core): Modified refundProcess to be extensible

[Feelw00]

Description

Unlike other state machines, the 'refund state' was previously implemented in a way that made extension impossible without modifying the core. I have modified it to allow for extension

• Discord
• Issue

Checklist

📌 Always:

• I have set a clear title
• My PR is small and contains a single feature
• I have checked my own PR

👍 Most of the time:

• I have added or updated test cases
• I have updated the README if needed

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	Jul 16, 2024 11:15am

[michaelbromley]

Hi!

Thank you, this is a very good PR. Not an easy one either as your first Vendure contribution - it's dealing with one of the more complex parts of the Vendure internals. But you've done it exactly as I would.

I have one reservation about merging this as-is though: I'm not really happy with creating a new refundOptions top-level config object just to store the refund process. I think I know why you did it that way - it is consistent with the other state machine configs <type>Options.process.

But logically I think that refund stuff belongs with the paymentOptions object - you cannot have a refund without a payment, they are part of the same larger process in fact. So in that respect I think it should go in paymentOptions. But then we have the problem of what to name the property. We of course can't use process because this is already taken by the payment process.

In hindsight I think I should have named it paymentProcess, then it would be trivial to just add refundProcess and everything is nice and consistent. But as it stands, I still feel that having process and refundProcess - although not exactly consistent - is still preferable to having a totally separate refundOptions object.

What are your thoughts on this?

[Feelw00]

@michaelbromley

Hi!

Thank you for your positive feedback on this PR.

As you mentioned, this PR extends the refund process by creating a top-level config object called refundOptions, aiming to maintain consistency with other state machine configurations.

I agree with your opinion, and I also believe that we should name it refundProcess and apply this naming convention to other state machine configurations in the future.

However, as you mentioned, this is my first Vendure contribution. Therefore, I thought it would be a better choice to create a refundOptions config object rather than changing the convention based on my personal judgment.

[michaelbromley]

OK here's what we do: remove the refundOptions, and add paymentOptions.refundProcess.

In a future release we will deprecate the process properties in favour of the more descriptive paymentProcess etc. which we can add at a later time.

[michaelbromley]

Thank you!

[Feelw00]

@michaelbromley

As discussed, I have added a commit to move refundProcess under paymentOptions. Renaming other state processes might be burdensome, but I am available to help if you decide to proceed with those changes.