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

docs: add ADR for fixed USD pricing #4395

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

zawan-ila
Copy link
Contributor

PROD-4124

ADR for fixed USD pricing in discovery.

Comment on lines 12 to 13
their introduction into discovery. Given the volatility of exchange rates, the prices coming into discovery may fluctuate with
each data ingestion.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the prices of same products coming into discovery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it to product prices as I feel it's better that way. Let me know if you think otherwise.

Comment on lines 15 to 18
The regular fluctuation in prices poses challenges for enterprises purchasing courses in bulk. These fluctuations can lead to
situations where an enterprise plans to make a purchase within budget, only to find that prices have changed unfavorably when
they proceed with the transaction. This inconsistency may disrupt budgetary planning and affect decision-making processes for
enterprise customers.
Copy link
Contributor

@DawoudSheraz DawoudSheraz Jul 25, 2024

Choose a reason for hiding this comment

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

rather than making it all about enterprise, we can use B2B terminology here. Furthermore, we can also add the impact on downstream reporting because of price changes with every ingestion. The current context is a bit too focused on Enterprise.
We should also focus on that because of predefined contracts, the price change is much more critical for businesses. This will also help in explanation of the solution of adding new field instead of updating the existing prices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to use B2B terminology. Also added that this is more critical for business customers.

I am not sure about the impacts on reporting part. I have not seen it being discussed as an issue in any of the docs I've come across. Could you provide more details here?


It would have been preferable to add this field at the Seat level somehow, and not clutter the CourseRun model unnecessarily.
However, due to some historical reasons, Executive Education courses (for which we need this functionality), only have unpaid
seats. It would be confusing to add pricing information to an unpaid seat.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also mention that in discovery, the pricing SoT is entitlement. Having a separate price field in Seat for CR would have been confusing and would have a major impact on the reporting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow the impacts on reporting here. Yes, in discovery the "normal" flow is to push entitlement prices to seats. However, I recall having seen PCs create course runs with different prices somehow 🤔 . I have not heard of that having a negative impact on the reporting.

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be better to verify this then.

Copy link
Contributor

@Ali-D-Akbar Ali-D-Akbar left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.


The existing pricing fields and their associated logic will remain unchanged within Course Discovery. It will be the responsibility
of consumers, such as enterprise, to appropriately manage any potential implications of using the fixed price field within their own
systems and processes.
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth mentioning that this field is specific to course runs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding to that, given the pricing SoT is Entitlement, we might want to add here why the field is not on Course level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it worth mentioning that this field is specific to course runs?

At line 22, I have mentioned that the field will be added to CourseRun model.

we might want to add here why the field is not on Course level

Can you suggest a sentence or two here? My understanding is that in the "SoT is Entitlement" model, it surely could have been. It is just that our business needs necessitate run level "pricing". I don't understand why the SoT was chosen to be the Course level in the first place.

Copy link
Contributor

@DawoudSheraz DawoudSheraz Jul 30, 2024

Choose a reason for hiding this comment

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

it surely could have been. It is just that our business needs necessitate run level "pricing".

Rather than saying "our" business needs, we can mention that ability to have prices different on course runs was not directly allowed and we needed to use coupons in e-commerce to change pricing for runs, etc. I understand not all context might be relevant here but considering how the price flow is currently, the information would be helpful in future for others learning more about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some context to this effect^ in the Alternatives section.

Comment on lines 43 to 44
However, due to some historical reasons, Executive Education courses (for which we need this functionality), only have unpaid
seats. It would be confusing to add pricing information to an unpaid seat.
Copy link
Contributor

Choose a reason for hiding this comment

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

Many parts of this information are 2U specific. We can re-word to explain the implication of seat level field and its impact on downstream consumers (LMS, Studio, E-comm) who all would need to cater to the field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[confused] The alternative I'm discussing here is to add a separate fixed_price_usd field at the seat level. The sync with different services for that field should not make it any worse as even our proposed solution has no sync flows.

Copy link
Contributor

Choose a reason for hiding this comment

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

The sync with different services for that field should not make it any worse as even our proposed solution has no sync flows.

Its not just sync, its ensuring the consumers are using that field appropriately. Ecomm and LMS have their versions of Seat models and they might require changes if the disco seat model is changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

consumers are using that field appropriately

Even the proposed approach leaves it to the consumers to pull the fixed_price information and make appropriate changes on their end. And does not force the fixed_price information upon anybody. We could have taken a similar approach where we add the fixed_price_information to Seat level but do not push it downstream.

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

I have left some small followup comments. Overall, this is good.

@zawan-ila zawan-ila requested a review from DawoudSheraz July 30, 2024 06:50
@zawan-ila zawan-ila merged commit 6e8c2b9 into openedx:master Jul 30, 2024
26 checks passed
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.

3 participants