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

OFFI-126: Adding several Stripe API endpoints and basics of Subscription #510

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

Conversation

wAsnk
Copy link
Contributor

@wAsnk wAsnk commented Nov 21, 2024

This comment has been minimized.

@@ -45,4 +47,4 @@
<PackageVersion Include="xunit" Version="2.8.0" />
<PackageVersion Include="xunit.runner.visualstudio" Version="2.8.0" />
</ItemGroup>
</Project>
</Project>
Copy link
Contributor

@sarahelsaig sarahelsaig Nov 22, 2024

Choose a reason for hiding this comment

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

why?
image

Suggested change
</Project>
</Project>

@@ -82,6 +84,18 @@ public int CompareTo(Amount other)
public Amount GetRounded() =>
new(Math.Round(Value, Currency.DecimalPlaces), Currency);

public long GetPaymentAmount(IEnumerable<string> zeroDecimalCurrencies, IEnumerable<string> specialCases)
Copy link
Contributor

@sarahelsaig sarahelsaig Nov 22, 2024

Choose a reason for hiding this comment

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

This is a very Stripe-specific thing (especially the "special cases") so as-is, it should be in StripePaymentIntentService instead of Amount.

Also the parameter names are not helpful. zeroDecimalCurrencies is not clear that it should be currency codes. Alternatively it could be a collection of ICurrency instead of string. Also specialCases is basically magic and again very Stripe-specific. If I wanted a long converting method in Amount, it would be something like this:

    /// <summary>
    /// Converts the <see cref="Amount"/> to a fixed-point fractional value by keeping some digits based on the <see
    /// cref="ICurrency.CurrencyIsoCode"/>.
    /// </summary>
    /// <param name="roundingByCurrencyCode">
    /// Provides exceptional rounding rules for currencies that aren't converted according to the default. The key is
    /// the <see cref="Currency"/>'s ISO code, the value pairs follow the same logic as the matching default parameters.
    /// </param>
    /// <param name="defaultKeepDigits">Indicates how many digits should be kept after the decimal point.</param>
    /// <param name="defaultRoundTens">
    /// If positive, the <see cref="Amount"/> is rounded to this many digits before converted to a fixed-point
    /// fractional. Ignored otherwise.
    /// </param>
    public long GetFixedPointAmount(
        IDictionary<string, (int KeepDigits, int RoundTens)> roundingByCurrencyCode,
        int defaultKeepDigits = 2,
        int defaultRoundTens = 0)
    {
        static int Tens(int zeroes) => (int)Math.Pow(10, zeroes);

        var (keepDigits, roundTens) = roundingByCurrencyCode.TryGetValue(Currency.CurrencyIsoCode, out var pair)
            ? pair
            : (defaultKeepDigits, defaultRoundTens);

        return roundTens > 0
            ? (long)Math.Round(Value / Tens(roundTens)) * Tens(roundTens + keepDigits)
            : (long)Math.Round(Value * Tens(keepDigits));
    }

And inside OrchardCore.Commerce.Payment.Stripe:

    public static long GetPaymentAmount(Amount total)
    {
        var rounding = ZeroDecimalCurrencies.Select(code => (Code: code, KeepDigits: 0, RoundTens: 0))
            .Concat(SpecialCases.Select(code => (Code: code, KeepDigits: 2, RoundTens: 2)))
            .ToDictionary(item => item.Code, item => (item.KeepDigits, item.RoundTens));

        return total.GetFixedPointAmount(rounding);
    }

validateUrl = 'checkout/validate/Stripe',
paramsUrl = 'checkout/params/Stripe',
validateUrl = 'checkout/validate/stripe',
paramsUrl = 'checkout/stripe/params',
Copy link
Contributor

Choose a reason for hiding this comment

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

All checkout-related addresses follow the ~/checkout/{action}/{providerName} formula so this change makes the API less consistent. If you really insist, rather replace it with ~/stripe/params without the checkout but it seems pointless to me.

Comment on lines +17 to +22
[Obsolete("This endpoint is obsolete and will be removed in a future version. Use checkout/stripe/middleware instead.")]
public static IEndpointRouteBuilder AddStripeMiddlewareEndpoint(this IEndpointRouteBuilder builder)
{
builder.MapPostWithDefaultSettings($"{StripePaymentApiPath}/middleware", AddStripeMiddlewareAsync);
return builder;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that the same the obsolete message says to use instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the files in this namespace are in the "Endpoints" directory while others are in the "EndPoints" directory.

image

This is bad form and it totally freaks out Rider, making some files fail to load. Looks like Orchard Core uses "Endpoints" with lower case "p", and the namespace here already matches that. So please move all files from "EndPoints" to "Endpoints". If Windows is giving you trouble with its case-insensitive file system, use the git mv.

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.

2 participants