Skip to content

Conversation

@mbertrand
Copy link
Member

@mbertrand mbertrand commented Sep 24, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5129

Description (What does it do?)

Adds a location field to LearningResource and LearningResourceRun and populates it for Sloan courses (locations for Professional Education courses will be handled after this one is merged in a separate PR). Location isn't relevant for other sources.

The value for LearningResource will be based on the value of the next/best LearningResourceRun.

How can this be tested?

  • Copy SEE_ env settings from RC
  • Run ./manage.py backpopulate_sloan_data
  • Run the following, you should get similar results:
    from learning_resources.models import *
    for location in LearningResource.objects.values_list("location", flat=True).distinct():
        print(location)
    
    'Cambridge, MA', 
    'Melbourne, Australia',
    'Lausanne, Switzerland',
    ...etc
  • The search page should still work, before and after running recreate_index --all

Additional Context

@mbertrand mbertrand force-pushed the mb/location branch 2 times, most recently from c203462 to 7e0f64d Compare October 4, 2024 20:05
@mbertrand mbertrand added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Oct 4, 2024
@abeglova abeglova self-assigned this Oct 7, 2024
model_name="learningresource",
name="location",
field=django.contrib.postgres.fields.ArrayField(
base_field=models.CharField(max_length=256), null=True, size=None
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good data structure. This makes the queries for questions like "List all the courses based in Cambridge, MA" or list "List all the locations which we have upcoming courses" needlessly complicated. Location should be a string field with a single location. Then either learningresourcerun should have an array column with a list of all the location ids for that run or (better) there should be a many to many learningresourcerun_locations table

Copy link
Contributor

Choose a reason for hiding this comment

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

It will also make it heard to catch if some locations are duplicates like "Cambridge, MA" vs "Cambridge MA" with no comma - although we don't have a good way of not creating duplicates anyway if Sloan lists the same locations as different strings

Copy link
Member Author

Choose a reason for hiding this comment

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

A learning resource run will only have 1 location. But since different runs may have different locations, I decided to make the learning resource location field an array of all the run values. But I can change it so that it is also a string field equal to the location of the next/best run, similar to availability and next_start_date, and calculated every night. I'm not too keen on a many-to-many relation between courses/runs and a new location model, since location is just a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be a string array field on learning resource then? I think my objection is having ['Cambridge, MA', 'Lausanne, Switzerland'] and just ['Cambridge, MA'] as seperate location records that multiple courses join to is bad

Copy link
Contributor

Choose a reason for hiding this comment

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

Also could ['Lausanne, Switzerland', 'Cambridge, MA'] potentially be created as a separate location record from ['Cambridge, MA', 'Lausanne, Switzerland']

Copy link
Member Author

Choose a reason for hiding this comment

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

Can this just be a string array field on learning resource then

Do you mean "Cambridge, MA; Lausanne, Switzerland" instead of ['Cambridge, MA', 'Lausanne, Switzerland'] for LearningResource.location? I could do that, or just set it to the one location of the next/best run. The location array is sorted in the ETL pipeline so there shouldn't be a ['Lausanne, Switzerland', 'Cambridge, MA'], it should always be ['Cambridge, MA', 'Lausanne, Switzerland'] or 'Cambridge, MA; Lausanne, Switzerland'

I can go ahead and make location a separate model with just a location charfield, with LearningResourceRun having a relation to just one Location object and LearningResource having either the same (identical to that of the next/best run) or a many-to-many relation (all run locations). But it's more joins on queries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored so that location is a CharField for both, and its value for LearningResource is set to the same as the next/best LearningResourceRun in learning_resources.etl.loaders.load_run_dependent_values

@abeglova abeglova added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Oct 7, 2024
@mbertrand mbertrand added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Oct 8, 2024
Copy link
Contributor

@abeglova abeglova left a comment

Choose a reason for hiding this comment

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

lgtm

@mbertrand mbertrand merged commit bdb534e into main Oct 8, 2024
11 checks passed
@odlbot odlbot mentioned this pull request Oct 8, 2024
5 tasks
jonkafton added a commit that referenced this pull request Oct 15, 2024
* Add location field to LearningResource and LearningResourceRun models (#1604)

* Always delete past events during ETL, and filter them out from api results too just in case (#1660)

* Release 0.21.1

* Release date for 0.21.1

* remove unnecessary padding adjustment on Input base styles (#1666)

* remove unnecessary padding adjustment on Input base styles

* adjust actual textarea padding to match Figma

* Content File Score Adjustment (#1667)

* Release 0.21.2

* Restore program letter intercept view (#1643)

* Restore program letter intercept view

* Generate schema

* Fix test

* fixing import

* Revert "fixing import"

This reverts commit cf56807.

---------

Co-authored-by: shankar ambady <ambady@mit.edu>

* Release date for 0.21.2

* topic detail banner / subtopic logic revisions (#1646)

* add new darker chip variant

* move subtopic display to the banner

* remove max width on banner subtitle texts

* query subtopics only on parent topic channels, and on child topic channels query "related topics" aka the topic's siblings

* don't display the current topic in related topics

* show parent topic breadcrumbs on child topic channel pages

* fix tests

* only do subtopics mocking if necessary

* fix editchannelpage test

* move action buttons under the title on mobile

* fix test

* add new default channel background image

* "toopics"

* fix background position

* remove accidentally committed file

* fix background size on mobile

* remove unnecessary order css rule

* fix banner background on mobile

* fix breadcrumb props

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove unnecessary topic id filter

* remove unnecessary breadcrumb fallback

* set the default banner background image in the banner component itself, and only apply large and up 140% background size if the default image is being used

* properly set default background image

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Update Yarn to v4.5.0 (#1624)

* Update Yarn to v4.5.0

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Clear Cache on Deploy (#1668)

* adding initial cache clear command

* adding cache clear command to heroku release script

* fix docstrings

* removing invalid flag from clear cache command (#1675)

* prevent featured course carousel from re-randomizing (#1673)

* use skipFeatured when invalidating userlists / learning paths after update

* add a new function for updating query data for user_list_parents and learning_path_parents on the featured courses list when either is edited without re-randomizing

* fix learning path invalidations

* fix test

* Shuffling around where the search_update event is fired so it happens in more places (#1679)

- Changed HeroSearch to use the SearchField component instead of SearchInput, so we get the event capture stuff
- Updated SearchInput to not require setPage (for use on the homepage)
- Moved facet events out of the SearchPage and into SearchDisplay so they get called on other pages that use it
- Also trigger the facet events on more things - should also now get sorting changes, choosing between categories, and clearing facets

* Reinstate background steps image

---------

Co-authored-by: Matt Bertrand <mrbertrand@gmail.com>
Co-authored-by: Doof <mitx-devops@mit.edu>
Co-authored-by: Carey P Gumaer <gumaerc@mit.edu>
Co-authored-by: Anastasia Beglova <abeglova@mit.edu>
Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com>
Co-authored-by: shankar ambady <ambady@mit.edu>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Shankar Ambady <shanbady@gmail.com>
Co-authored-by: James Kachel <james@jkachel.com>
jonkafton added a commit that referenced this pull request Oct 21, 2024
* Merge `main` into `nextjs` (#1687)

* Add location field to LearningResource and LearningResourceRun models (#1604)

* Always delete past events during ETL, and filter them out from api results too just in case (#1660)

* Release 0.21.1

* Release date for 0.21.1

* remove unnecessary padding adjustment on Input base styles (#1666)

* remove unnecessary padding adjustment on Input base styles

* adjust actual textarea padding to match Figma

* Content File Score Adjustment (#1667)

* Release 0.21.2

* Restore program letter intercept view (#1643)

* Restore program letter intercept view

* Generate schema

* Fix test

* fixing import

* Revert "fixing import"

This reverts commit cf56807.

---------

Co-authored-by: shankar ambady <ambady@mit.edu>

* Release date for 0.21.2

* topic detail banner / subtopic logic revisions (#1646)

* add new darker chip variant

* move subtopic display to the banner

* remove max width on banner subtitle texts

* query subtopics only on parent topic channels, and on child topic channels query "related topics" aka the topic's siblings

* don't display the current topic in related topics

* show parent topic breadcrumbs on child topic channel pages

* fix tests

* only do subtopics mocking if necessary

* fix editchannelpage test

* move action buttons under the title on mobile

* fix test

* add new default channel background image

* "toopics"

* fix background position

* remove accidentally committed file

* fix background size on mobile

* remove unnecessary order css rule

* fix banner background on mobile

* fix breadcrumb props

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove unnecessary topic id filter

* remove unnecessary breadcrumb fallback

* set the default banner background image in the banner component itself, and only apply large and up 140% background size if the default image is being used

* properly set default background image

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Update Yarn to v4.5.0 (#1624)

* Update Yarn to v4.5.0

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Clear Cache on Deploy (#1668)

* adding initial cache clear command

* adding cache clear command to heroku release script

* fix docstrings

* removing invalid flag from clear cache command (#1675)

* prevent featured course carousel from re-randomizing (#1673)

* use skipFeatured when invalidating userlists / learning paths after update

* add a new function for updating query data for user_list_parents and learning_path_parents on the featured courses list when either is edited without re-randomizing

* fix learning path invalidations

* fix test

* Shuffling around where the search_update event is fired so it happens in more places (#1679)

- Changed HeroSearch to use the SearchField component instead of SearchInput, so we get the event capture stuff
- Updated SearchInput to not require setPage (for use on the homepage)
- Moved facet events out of the SearchPage and into SearchDisplay so they get called on other pages that use it
- Also trigger the facet events on more things - should also now get sorting changes, choosing between categories, and clearing facets

* Reinstate background steps image

---------

Co-authored-by: Matt Bertrand <mrbertrand@gmail.com>
Co-authored-by: Doof <mitx-devops@mit.edu>
Co-authored-by: Carey P Gumaer <gumaerc@mit.edu>
Co-authored-by: Anastasia Beglova <abeglova@mit.edu>
Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com>
Co-authored-by: shankar ambady <ambady@mit.edu>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Shankar Ambady <shanbady@gmail.com>
Co-authored-by: James Kachel <james@jkachel.com>

* restoring up and down chevrons (#1690)

* Config to set cache control headers (#1700)

* Middleware to set cache headers

* Set headers in the config

* Upgrade Next.js

* Regex to exclude paths with a file extension to apply only to routes

* Remove middleware file

* Instruct to only store in shared cache (not browsers)

* Copy yarn releases to the Docker container (fixes build) (#1703)

* Copy yarn releases

* Build the docker image on all push

* NextJS - re-enable program letter tests (#1696)

* fixing program letter template

* re-enabling test

* adding space

* remove unnecessary webpack customizations (#1704)

* Increase cache duration (#1705)

* Increase cache durations

* Update comment

* Prod deployment. Add Posthog vars

* NextJS Sentry Integration (#1701)

* configure sentry, fix error pages

* add env vars to CI

* add global styles to global-error

* TEMPORARY self destruct button

* cleanup

* cleanup error boundary, other misc cleanup

* remove a few useless things

* add a simple test i guess

* remove intentional error triggers

* Appzi env vars and Sentry config

* Move deploy jobs to respective workflow. Docker build for dry run

---------

Co-authored-by: Matt Bertrand <mrbertrand@gmail.com>
Co-authored-by: Doof <mitx-devops@mit.edu>
Co-authored-by: Carey P Gumaer <gumaerc@mit.edu>
Co-authored-by: Anastasia Beglova <abeglova@mit.edu>
Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com>
Co-authored-by: shankar ambady <ambady@mit.edu>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Shankar Ambady <shanbady@gmail.com>
Co-authored-by: James Kachel <james@jkachel.com>
Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com>
@mbertrand mbertrand deleted the mb/location branch October 23, 2024 12:49
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