Skip to content

Conversation

@mbertrand
Copy link
Member

@mbertrand mbertrand commented Oct 24, 2024

What are the relevant tickets?

PR 1 of 5 to address https://github.com/mitodl/hq/issues/5134 and https://github.com/mitodl/hq/issues/4685

Description (What does it do?)

  • Adds a LearningResourcePrice model with two fields (amount, currency)
  • Adds a many-to-many relation between LearningResource and LearningResourcePrice, also between LearningResourceRun and LearningResourcePrice (named resource_prices)
  • Updates QuerySet classes to avoid n+1 queries
  • Updates the search index schema with addition of the new resource_prices field
  • Adds a new python package pycountry to validate currency codes during ETL pipeline runs

How can this be tested?

  • If you don't already have some courses with prices, run these commands on the main branch:
    ./manage.py backpopulate_mitxonline_data
    ./manage.py backpopulate_xpro_data
    
  • Log in as an admin user to avoid caching and go to http://open.odl.local:8062/search?platform=mitxonline&platform=xpro, you will see a mix of courses that should all either be paid with certificate (xpro) or free with a paid certificate option (mitxonline). If you click on a course card you will see the same price in the drawer.
  • Switch to this branch, run docker compose build web celery (new python package) and restart containers. Make sure the new migrations complete successfully.
  • Go to http://open.odl.local:8062/search?platform=mitxonline&platform=xpro again. Everything should still look the same.
  • Click on a result and check the API response for that resource. There should be a resource_prices attribute for the resource and each of it's runs, with amount and currency attributes.
  • Run ./manage.py update_index --courses --programs, then wait a minute or 2.
  • Go to http://open.odl.local:8062/search?platform=mitxonline&platform=xpro again. It should still look the same, and if you inspect search API results, the resource_prices field should be there too.
  • Run ./manage.py recreate_index --all, it should complete without errors.
  • Check and uncheck the "Free" checkbox on the search page. The results should change as expected.
  • Running the following pipelines should still complete successfully (you'll need the appropriate env values from RC for SEE_* and EDX_API_*):
    ./manage.py backpopulate_sloan_data
    ./manage.py backpopulate_mit_edx_data [--api_course_file=edx.json --api_program_datafile=edx_programs.json]
    
  • Use the django debug toolbar to measure the # and speed of SQL queries for the URL http://api.open.odl.local:8063/api/v1/learning_resources/?platform=mitxonline. It should not be significantly slower than the same url on the main branch, but there will be 2 additional queries.
Screenshot 2024-10-23 at 1 59 26 PM

vs

Screenshot 2024-10-23 at 2 01 03 PM
  • Finally if you want to revert your price data before you go back to the main branch, run this and the prices field should be restored with values:
    ./manage.py migrate learning_resources 0070_learningresource_location
    

Additional Context

  • Most data sources always have prices in USD. Exceptions are Sloan and MIT edX, so their ETL pipelines have been adjusted to extract the currency from their API's.
  • There will be 4 subsequent PR's before https://github.com/mitodl/hq/issues/5134 is closed:
    • 2: Change frontend to use the resource_prices field instead of prices
    • 3: Modify the LearningResource.prices and LearningResourceRun.prices fields to be the same as resource_prices, and update schema. (Note: this will cause index updates to fail until a reindex completes)
    • 4: Change frontend to use prices field again
    • 5: Remove resource_prices from models and schemas.

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

This looks good 👍

On thing I noticed is that when I ran the indexing commands, Django told me:

WARNINGS:
learning_resources.LearningResource.resource_prices: (fields.W340) null has no effect on ManyToManyField.
learning_resources.LearningResourceRun.resource_prices: (fields.W340) null has no effect on ManyToManyField.
Operations to perform

@mbertrand
Copy link
Member Author

@ChristopherChudzicki Good catch, I just updated the model definition/migration for those fields

@mbertrand mbertrand force-pushed the mb/price_model_01 branch 2 times, most recently from 7065765 to 94b6e25 Compare October 25, 2024 15:47
@mbertrand mbertrand merged commit 18d6ac6 into main Oct 28, 2024
11 checks passed
This was referenced Oct 28, 2024
@rhysyngsun rhysyngsun deleted the mb/price_model_01 branch February 7, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants