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

removed global config and static classes #47

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

olokken
Copy link
Contributor

@olokken olokken commented Jul 17, 2023

No description provided.

Copy link

@OFollan OFollan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor question 😄 And some rambling to myself that can be ignored

@olokken olokken requested review from aslaksm and OFollan July 18, 2023 10:51
Copy link

@okpedersen okpedersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of extra notes:

  • Vipps.net.AspDemo is not updated
  • Vipps.net.csproj version field should be bumped (0.9.0 is a good new version since the PR introduces breaking changes)

src/Vipps.net/Services/AccessTokenCacheService.cs Outdated Show resolved Hide resolved
src/Vipps.net.AspCoreDemo/Startup.cs Outdated Show resolved Hide resolved
src/Vipps.net.AspCoreDemo/Startup.cs Outdated Show resolved Hide resolved
return this._accessTokenService;
}

public IVippsEpaymentService EpaymentService()
Copy link

@sebdak sebdak Jul 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for instantiating the services through functions? Can't they live as members of the VippsApi class, instantiated by the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not anymore, I changed this now. Also did some changes to startup in the Vipps.net.AspCoreDemo, to use the IOptions pattern instead of injecting singleton.

}

public Uri BaseAddress
{
get { return HttpClient.BaseAddress; }
get { return new Uri(UrlHelper.GetBaseUrl(_options.UseTestMode)); }
Copy link
Contributor

@SvenErikPy SvenErikPy Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should return the actual HttpClients base address, as a custom HttpClient with a non-default base address can be provided in the constructor. (For example by us to test UAT)

@@ -6,26 +6,44 @@

namespace Vipps.net.Services
{
public static class AccessTokenService
public interface IVippsAccessTokenService
Copy link
Contributor

@SvenErikPy SvenErikPy Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just forgot to rename the file, the file name should be the same as the interface/class (VippsAccessTokenService)

@@ -6,34 +6,53 @@

namespace Vipps.net.Services
{
public static class CheckoutService
public interface IVippsCheckoutService
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And VippsCheckoutService

private readonly IVippsEpaymentService _epaymentService;
private readonly IVippsCheckoutService _checkoutService;

public VippsApi(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should provide a way to inject a custom HttpClient (if we want to keep the feature, which I assume we are?)

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.

6 participants