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

Adds pricePerPeriod functions to PricingPhase #1789

Merged
merged 12 commits into from
Jul 26, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,14 @@ static void checkPricingPhase(PricingPhase pricingPhase) {
RecurrenceMode recurrenceMode = pricingPhase.getRecurrenceMode();
Integer billingCycleCount = pricingPhase.getBillingCycleCount();
Price price = pricingPhase.getPrice();
String pricePerMonth = pricingPhase.formattedPriceInMonths(locale);
String pricePerMonthNoLocale = pricingPhase.formattedPriceInMonths();
String pricePerMonthString = pricingPhase.formattedPriceInMonths(locale);
String pricePerMonthStringNoLocale = pricingPhase.formattedPriceInMonths();
Price pricePerWeek = pricingPhase.pricePerWeek(locale);
Price pricePerMonth = pricingPhase.pricePerMonth(locale);
Price pricePerYear = pricingPhase.pricePerYear(locale);
Price pricePerWeekNoLocale = pricingPhase.pricePerWeek();
Price pricePerMonthNoLocale = pricingPhase.pricePerMonth();
Price pricePerYearNoLocale = pricingPhase.pricePerYear();

OfferPaymentMode offerPaymentMode = pricingPhase.getOfferPaymentMode();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,14 @@ private class PricingPhaseAPI {
val recurrenceMode: RecurrenceMode = pricingPhase.recurrenceMode
val billingCycleCount: Int? = pricingPhase.billingCycleCount
val price: Price = pricingPhase.price
val pricePerMonth: String = pricingPhase.formattedPriceInMonths(locale)
val pricePerMonthNoLocale: String = pricingPhase.formattedPriceInMonths()
val pricePerMonthString: String = pricingPhase.formattedPriceInMonths(locale)
val pricePerMonthStringNoLocale: String = pricingPhase.formattedPriceInMonths()
val pricePerWeek = pricingPhase.pricePerWeek(locale)
val pricePerMonth = pricingPhase.pricePerMonth(locale)
val pricePerYear = pricingPhase.pricePerYear(locale)
val pricePerWeekNoLocale = pricingPhase.pricePerWeek()
val pricePerMonthNoLocale = pricingPhase.pricePerMonth()
val pricePerYearNoLocale = pricingPhase.pricePerYear()

val offerPaymentMode: OfferPaymentMode? = pricingPhase.offerPaymentMode
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package com.revenuecat.purchases.models

import android.os.Parcelable
import com.revenuecat.purchases.utils.pricePerMonth
import com.revenuecat.purchases.utils.pricePerWeek
import com.revenuecat.purchases.utils.pricePerYear
import kotlinx.parcelize.Parcelize
import java.util.Locale

Expand Down Expand Up @@ -54,14 +56,54 @@ data class PricingPhase(
}
}

/**
* Gives the price of the [PricingPhase] in the given locale in a weekly recurrence. This means that for example,
* if the period is monthly, the price will be divided by 4. It uses a currency formatter to format the price in
* the given locale. Note that this value may be an approximation.
* @param locale Locale to use for formatting the price. Default is the system default locale.
*/
@JvmOverloads
fun pricePerWeek(locale: Locale = Locale.getDefault()): Price =
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised codecov says these are not tested when we are adding tests in this PR 🤔. Not sure what's happening though... Any ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's very weird. I'm investigating. 🔍

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured it out! I wasn't testing the overloads without Locale! 46b8d7a

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh makes sense. Thanks for debugging that!

price.pricePerWeek(billingPeriod, locale)

/**
* Gives the price of the [PricingPhase] in the given locale in a monthly recurrence. This means that for example,
* if the period is annual, the price will be divided by 12. It uses a currency formatter to format the price in
* the given locale. Note that this value may be an approximation.
* @param locale Locale to use for formatting the price. Default is the system default locale.
*/
@JvmOverloads
fun pricePerMonth(locale: Locale = Locale.getDefault()): Price =
price.pricePerMonth(billingPeriod, locale)

/**
* Gives the price of the [PricingPhase] in the given locale in a yearly recurrence. This means that for example,
* if the period is monthly, the price will be multiplied by 12. It uses a currency formatter to format the price in
* the given locale. Note that this value may be an approximation.
* @param locale Locale to use for formatting the price. Default is the system default locale.
*/
@JvmOverloads
fun pricePerYear(locale: Locale = Locale.getDefault()): Price =
price.pricePerYear(billingPeriod, locale)

/**
* Gives the price of the [PricingPhase] in the given locale in a monthly recurrence. This means that for example,
* if the period is annual, the price will be divided by 12. It uses a currency formatter to format the price in
* the given locale. Note that this value may be an approximation.
*
* This is equivalent to:
* ```kotlin
* pricePerMonth(locale).formatted
* ```
*
* @param locale Locale to use for formatting the price. Default is the system default locale.
*/
@Deprecated(
message = "pricePerMonth() provides more price info",
replaceWith = ReplaceWith("pricePerMonth(locale).formatted"),
)
@JvmOverloads
fun formattedPriceInMonths(locale: Locale = Locale.getDefault()): String {
return price.pricePerMonth(billingPeriod, locale).formatted
return pricePerMonth(locale).formatted
Comment on lines 106 to +107
Copy link
Member Author

@JayShortway JayShortway Jul 24, 2024

Choose a reason for hiding this comment

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

Should we deprecate formattedPriceInMonths() and direct people to use pricePerMonth().formatted directly? What do you think? It might be confusing that 2 methods do pretty much the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree with that. formattedPriceInMonths doesn't really fit the naming we've been using 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, done: 5947b8b

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
package com.revenuecat.purchases.common.models

import com.revenuecat.purchases.models.Period
import com.revenuecat.purchases.models.Price
import com.revenuecat.purchases.models.PricingPhase
import com.revenuecat.purchases.models.RecurrenceMode
import org.assertj.core.api.Assertions.assertThat
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.Parameterized
import java.util.Locale

@RunWith(Parameterized::class)
class ParameterizedPricingPhaseTest(
private val billingPeriodIso8601: String,
private val expected: Expected,
) {

class Expected(
val weekly: Price,
val monthly: Price,
val yearly: Price,
)

companion object {

private val BASE_PRICE = Price(
formatted = "$99.99",
amountMicros = 99990000,
currencyCode = "USD",
)

@Suppress("LongMethod")
@JvmStatic
@Parameterized.Parameters(name = "{0}")
fun parameters(): Collection<*> = listOf(
arrayOf(
"P1W",
Expected(
weekly = BASE_PRICE,
monthly = Price(
formatted = "$434.48",
amountMicros = 434480357,
currencyCode = "USD"
),
yearly = Price(
formatted = "$5,213.76",
amountMicros = 5213764285,
currencyCode = "USD",
)
)
),
arrayOf(
"P1M",
Expected(
weekly = Price(
formatted = "$23.01",
amountMicros = 23011397,
currencyCode = "USD",
),
monthly = BASE_PRICE,
yearly = Price(
formatted = "$1,199.88",
amountMicros = 1199880000,
currencyCode = "USD",
)
)
),
arrayOf(
"P1Y",
Expected(
weekly = Price(
formatted = "$1.92",
amountMicros = 1917616,
currencyCode = "USD",
),
monthly = Price(
formatted = "$8.33",
amountMicros = 8332500,
currencyCode = "USD",
),
yearly = BASE_PRICE
)
),
)
}

@Suppress("DEPRECATION")
@Test
fun `should correctly calculate periodic prices for billing period`() {
// Arrange
val phase = PricingPhase(
billingPeriod = Period.create(billingPeriodIso8601),
recurrenceMode = RecurrenceMode.FINITE_RECURRING,
billingCycleCount = 2,
price = BASE_PRICE
)

// Act
val actualWeekly = phase.pricePerWeek(Locale.US)
val actualMonthly = phase.pricePerMonth(Locale.US)
val actualYearly = phase.pricePerYear(Locale.US)
val actualWeeklyNoLocale = phase.pricePerWeek()
val actualMonthlyNoLocale = phase.pricePerMonth()
val actualYearlyNoLocale = phase.pricePerYear()
val actualMonthlyFormatted = phase.formattedPriceInMonths(Locale.US)
val actualMonthlyFormattedNoLocale = phase.formattedPriceInMonths()

// Assert
assertThat(actualWeekly).isEqualTo(expected.weekly)
assertThat(actualMonthly).isEqualTo(expected.monthly)
assertThat(actualYearly).isEqualTo(expected.yearly)
assertThat(actualWeeklyNoLocale.amountMicros).isEqualTo(expected.weekly.amountMicros)
assertThat(actualMonthlyNoLocale.amountMicros).isEqualTo(expected.monthly.amountMicros)
assertThat(actualYearlyNoLocale.amountMicros).isEqualTo(expected.yearly.amountMicros)
assertThat(actualMonthlyFormatted).isEqualTo(expected.monthly.formatted)
// Hard to test anything else if the locale is not deterministic, besides formatting exactly like the
// implementation does.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could mock the Locale.getDefault static method... But I don't think it's a big deal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, true! I don't think it's super important either though, because this is deprecated, and the implementation it calls is fully tested.

assertThat(actualMonthlyFormattedNoLocale).isNotBlank()
}
}