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

Refactor for future development #8

Open
wants to merge 15 commits into
base: support/10.x
Choose a base branch
from

Conversation

PeterKvayt
Copy link

Hi @umbracotrd!

As I can see you have merged my previous pr. But I did not confired it yet as you asked.

So here is new changes and you can fix problem with version.

Could you please increase minor version (to 10.1.0.0), so other developers should be aware to review changes before use new version.

Thanks.

PeterKvayt and others added 11 commits July 27, 2024 22:50
# Conflicts:
#	src/Umbraco.Commerce.PaymentProviders.Worldpay/Constants/WorldpayValues.cs
#	src/Umbraco.Commerce.PaymentProviders.Worldpay/Extensions/PaymentStatusExtensions.cs
#	src/Umbraco.Commerce.PaymentProviders.Worldpay/WorldpayBusinessGateway350PaymentProvider.cs
#	src/Umbraco.Commerce.PaymentProviders.Worldpay/WorldpayBusinessGateway350Settings.cs
@umbracotrd
Copy link
Contributor

Hi @PeterKvayt , as I understand, this PR is just for refactoring code, right? I'm inclined to do this kind of things on v13 and above instead of v10. What do you think about this @mattbrailsford ?

@PeterKvayt
Copy link
Author

Hi @umbracotrd !

Yea, this is just refactoring, In some places log level/messages were changed for better log history.

@mattbrailsford
Copy link
Contributor

V13+ is cool with me. V10 is security fixes only now which this doesn’t look to fall into.

@PeterKvayt PeterKvayt changed the title Temp/4 Draft:Temp/4 Aug 22, 2024
@PeterKvayt PeterKvayt marked this pull request as draft August 22, 2024 13:33
@PeterKvayt PeterKvayt changed the title Draft:Temp/4 Temp/4 Aug 22, 2024
@VictorTavkin
Copy link

VictorTavkin commented Oct 15, 2024

Hi everyone!

@mattbrailsford, @umbracotrd,

I would like to request your assistance in merging the proposed changes into v10. I completely understand that the version is currently in the support phase and it should not include any improvements and refactoring. @PeterKvayt and I have invested a significant amount of time in setting up the plugin, investigating/fixing the issues, and refactoring, but got the situation below.

We’ve recently gone live with Umbraco v10 and encountered a few issues related to webhooks and logging, such as:

  1. Receiving two callbacks from Worldpay for a single order: one from MC_callbackurl and another from PaymentResponse (requires investigation).
  2. An error entry appears in the log files when a user cancels an order (the fix in the current PR).
  3. Probably, there will be other issues in the future.

So, taking into account, the information above, now, we are in a situation where we can't further contribute to the plugin in v10, because the current PR is missing in that version.
Plus, we can't upgrade the website Umbraco version to v13+ to contribute to the version where the current PR is accepted.
So we can't develop either v10 or v13+.

Additionally, given the current situation, two separate fixes are needed:

  • one for v10
  • another for v13+
    This will take twice as long because a regular merge is not possible due to the absence of the current PR in v10

Thank you for your time and consideration.

@PeterKvayt PeterKvayt marked this pull request as ready for review October 15, 2024 08:06
@umbracotrd
Copy link
Contributor

umbracotrd commented Oct 16, 2024

Hi @VictorTavkin and @PeterKvayt , I appreciate your effort to contribute to this package. It's always nice to have more support from community.
Merging this PR for v10 seems fine for me, but it contains refactored code which makes it hard to cherry pick to v12, 13 and 14.
Let me bring this up to the team and get back to you.

@umbracotrd umbracotrd changed the title Temp/4 Refactor for future development Oct 16, 2024
@umbracotrd
Copy link
Contributor

Hi @VictorTavkin and @PeterKvayt , I've pushed version 10.1.0--preview.2+61ae4c6 to Umbraco Nightly Build feed. Could you confirm that it works as you expect?

We'll find a way to merge the changes up to newer versions later.

@VictorTavkin
Copy link

Hi,
@mattbrailsford, @umbracotrd,
Thank you for your help. We'll return once we've tested the latest version.

@umbracotrd
Copy link
Contributor

Hi @VictorTavkin, how's it going with the 10.1.0 nightly build? Do you need more time to test it?

@VictorTavkin
Copy link

Hi @umbracotrd,
We plan to deploy the nightly build to our staging environment this week. then we'll get back to you as soon as we finish testing.

@PeterKvayt
Copy link
Author

@mattbrailsford
@umbracotrd
@VictorTavkin

I found issue in current pr, soon I will push fix.

In GetOrderReferenceAsync method context.Order is null, so we can't get OrderNumber from it.
@PeterKvayt
Copy link
Author

@umbracotrd
@mattbrailsford

Please, do not push new nightly build until I or @VictorTavkin do not ask about it.

@PeterKvayt
Copy link
Author

This commit fixes NullreferenceException when verbosity logging is enabled.

This commit adds ability to disable custom form fields. It is can be useful for cases when you use MC_callbackurl instead of Payment Response Url (field in Worldpay management dashboard).
We have conversation with Worldpay and they told us that we can use one of the folloing options to provide callback url: MC_callbackurl or Payment Response Url. To test this, we need to add ability to enable/disable MC_callbackurl option.

@umbracotrd @mattbrailsford Could you please push new nightly build?

@umbracotrd
Copy link
Contributor

@PeterKvayt : I've pushed 10.1.0--preview.4+0603135 to Umbraco nightly feed

@PeterKvayt
Copy link
Author

@PeterKvayt : I've pushed 10.1.0--preview.4+0603135 to Umbraco nightly feed

@umbracotrd Many thanks! We will aplly new 10.1.0--preview.4+0603135 as soon as possible.

@PeterKvayt
Copy link
Author

We already applied 10.1.0--preview.4+0603135 and now it is under testing.

@VictorTavkin
Copy link

Hi!

Sorry for a delay with a feedback - the setup of production environment for Worldpay required some extra work and adjustments for our website plus a review of the setup from Worldpay's team. Therefore it took more time for setup and testing than it was initially expected.

Anyway, finally, we payed for an order using Worldpay payment method on production environment.

Many thanks for your patience and support! Please, merge the current PR. From our side no further changes are expected.

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.

4 participants