Move all services to BasicService #1305

remi-stripe · 2018-09-29T01:14:38Z

This PR moves all existing services over to BasicService.

cc @stripe/api-libraries

remi-stripe

Comments that came to mind while porting everything.

remi-stripe · 2018-09-29T01:16:50Z

src/Stripe.net/Services/ApplicationFees/ApplicationFeeService.cs

+
+        public virtual StripeList<ApplicationFee> List(ApplicationFeeListOptions options = null, RequestOptions requestOptions = null)
+        {
+            return this.GetEntityList($"{Urls.BaseUrl}/application_fees", requestOptions, options);
        }

        public virtual ApplicationFee Refund(string applicationFeeId, int? refundAmount = null, RequestOptions requestOptions = null)


Should we just remove the Refund method on this service and force devs to use the ApplicationFeeRefundService instead? If not, should we make this method take an Options class instead of an amount?

👍 on removing the method.

remi-stripe · 2018-09-29T01:17:41Z

src/Stripe.net/Services/Topups/TopupService.cs

@@ -21,87 +21,52 @@ public TopupService(string apiKey = null)

        public virtual Topup Cancel(string topupId, RequestOptions requestOptions = null)


Cancel has no parameters today but might in the future. What is the best approach? Adding it in the future would be a breaking change (unless we add an overloaded method).

Yeah, I suppose we could make it a convention that all non-standard request methods (i.e. request methods not covered by the CRUD + list interfaces) must take an options argument, even if the options class is empty.

remi-stripe · 2018-09-29T01:18:18Z

src/Stripe.net/Services/Issuing/Cards/CardService.cs

-                Requestor.PostString(
-                    this.ApplyAllParameters(createOptions, classUrl, false),
-                    this.SetupRequestOptions(requestOptions)));
+            return this.Post($"{classUrl}", requestOptions, options);
        }

        public virtual CardDetails Details(string cardId, RequestOptions requestOptions = null)


This method still uses the old way since it returns CardDetails and not Card. What is the right approach? The current method is verbose and error prone IMO but not sure there's a better way

I think this is fine for now. I have some ideas for covering these cases using generic methods (but will be much simpler to experiment once this PR is merged).

remi-stripe · 2018-09-29T01:19:03Z

src/Stripe.net/Services/Invoices/InvoiceService.cs

        }

-        public virtual async Task<Invoice> GetAsync(string invoiceId, RequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken))
+        public virtual StripeList<InvoiceLineItem> ListLineItems(string invoiceId, InvoiceListLineItemsOptions listOptions = null, RequestOptions requestOptions = null)


Same as Issuing.Card: This returns a different class so we can't use the GetEntity approach. What is the right way to do this? Should it be in the InvoiceLineItemService (does not yet exist)?

Let's leave it as is for now.

remi-stripe · 2018-09-29T01:19:53Z

src/Stripe.net/Services/BankAccounts/BankAccountService.cs

-                Requestor.PostString(
-                    this.ApplyAllParameters(createOptions, $"{Urls.BaseUrl}/customers/{customerId}/bank_accounts"),
-                    this.SetupRequestOptions(requestOptions)));
+            return this.Post($"{Urls.BaseUrl}/customers/{customerId}/bank_accounts", requestOptions, options);


Should we stop using /bank_accounts and move to /sources like we do with cards? I think this will require a default value for the options class but might be cleaner.
Issue is that stripe-mock will not return a bank account though not a big deal I guess.

👍 on using /sources.

remi-stripe · 2018-09-29T01:20:20Z

src/Stripe.net/Services/OAuth/OAuthTokenService.cs

@@ -4,7 +4,7 @@ namespace Stripe
    using System.Threading.Tasks;
    using Stripe.Infrastructure;

-    public class OAuthTokenService : StripeService,
+    public class OAuthTokenService : BasicService<OAuthToken>,


This class has no OAuthTokenServiceTest we should fix this as it makes it dangerous to make random changes

Agree with the sentiment, but in practice we'd need local mocking to test this (stripe-mock doesn't support OAuth endpoints).

remi-stripe · 2018-09-29T01:20:41Z

src/Stripe.net/Services/OAuth/OAuthTokenService.cs

        }

        public virtual OAuthDeauthorize Deauthorize(string clientId, string stripeUserId, RequestOptions requestOptions = null)
        {
-            var url = ParameterBuilder.ApplyParameterToUrl(Urls.OAuthDeauthorize, "client_id", clientId);
+            var url = ParameterBuilder.ApplyParameterToUrl($"{Urls.BaseConnectUrl}/oauth/deauthorize", "client_id", clientId);


Same as Issuing.Card: returns a different object

remi-stripe · 2018-09-29T01:21:05Z

src/Stripe.net/Services/OAuth/OAuthTokenService.cs

+
+        public virtual Task<OAuthToken> CreateAsync(OAuthTokenCreateOptions options, RequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken))
+        {
+            return this.PostAsync($"{Urls.BaseConnectUrl}/oauth/token", requestOptions, cancellationToken, options);
        }

        public virtual OAuthDeauthorize Deauthorize(string clientId, string stripeUserId, RequestOptions requestOptions = null)


Why not a OAuthDeauthorizeOptions or something?

Maybe, but OAuth is definitely an odd case. I wouldn't sweat it if it doesn't abide completely by the library's conventions.

remi-stripe · 2018-09-29T01:21:40Z

src/Stripe.net/Services/Sources/SourceService.cs

@@ -95,7 +95,7 @@ public virtual async Task<Source> DetachAsync(string customerId, string sourceId

        public virtual async Task<StripeList<Source>> ListAsync(string customerId, SourceListOptions listOptions = null, RequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken))


Do you know why we don't simply use the GetEntityList and DeleteEntity on this service?

I think we couldn't use DeleteEntity before because it returned a StripeDeleted object, but detaching a source always returned a source.

Not sure about GetEntityList. We can likely use it as well.

remi-stripe · 2018-09-29T14:13:05Z

@ob-stripe Okay I did another round of fixes. There are 2 Services left that use Requestor.* directly: OAuthTokenService and FileService. My guess is that we can fix the first one and not the latter. Can you have a look?

ob-stripe · 2018-09-29T15:12:59Z

My guess is that we can fix the first one and not the latter. Can you have a look?

Yeah, that sounds correct. I updated OAuthTokenService to use the new generic methods.

Doing the same for FileService would require some more refactoring, but I don't think it's worth it for now since only file creation requests would benefit from it. Let's leave as is for now.

remi-stripe assigned ob-stripe Sep 29, 2018

remi-stripe changed the base branch from master to integration-next-major-version September 29, 2018 01:15

remi-stripe commented Sep 29, 2018

View reviewed changes

remi-stripe force-pushed the remi-move-all-services branch from d669ed7 to 4346480 Compare September 29, 2018 14:12

remi-stripe assigned ob-stripe and unassigned ob-stripe Sep 29, 2018

ob-stripe approved these changes Sep 29, 2018

View reviewed changes

ob-stripe added the approved label Sep 29, 2018

ob-stripe changed the title ~~[WIP] Move all services to BasicService~~ Move all services to BasicService Sep 29, 2018

ob-stripe mentioned this pull request Sep 29, 2018

stripe-dotnet v20.0 #1293

Merged

32 tasks

Move all services to BasicService

d450f4c

ob-stripe force-pushed the remi-move-all-services branch from 87027dc to d450f4c Compare September 29, 2018 15:16

ob-stripe merged commit 542842c into integration-next-major-version Sep 29, 2018

ob-stripe deleted the remi-move-all-services branch September 29, 2018 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all services to BasicService #1305

Move all services to BasicService #1305

remi-stripe commented Sep 29, 2018

remi-stripe left a comment

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe commented Sep 29, 2018

ob-stripe commented Sep 29, 2018

		@@ -21,87 +21,52 @@ public TopupService(string apiKey = null)

		public virtual Topup Cancel(string topupId, RequestOptions requestOptions = null)

		@@ -95,7 +95,7 @@ public virtual async Task<Source> DetachAsync(string customerId, string sourceId

		public virtual async Task<StripeList<Source>> ListAsync(string customerId, SourceListOptions listOptions = null, RequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken))

Move all services to BasicService #1305

Move all services to BasicService #1305

Conversation

remi-stripe commented Sep 29, 2018

remi-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remi-stripe commented Sep 29, 2018

ob-stripe commented Sep 29, 2018