mitodl / open-discussions

BSD 3-Clause "New" or "Revised" License
10 stars 2 forks source link

Userlist `image_src` is invalid for lists that contain OCW resources #3836

Open ChristopherChudzicki opened 1 year ago

ChristopherChudzicki commented 1 year ago

I describe this bug for learning lists at /learn/lists. We are migrating that functionality to the TS codebase at /infinite/lists. But fundamentally I believe this an issue with the backend API, so I'm not addressing it in #3819

Steps to Reproduce

  1. Visit https://discussions-rc.odl.mit.edu/learn/search
  2. Find any OCW resource
  3. click the bookmark icon on the resource
  4. Create a new, empty learning list
  5. Add the OCW resource to the newly created list
  6. View the list on the lists page: https://discussions-rc.odl.mit.edu/learn/lists

Expected Behavior

List cover image shows up

Actual Behavior

List cover image is blank

Screenshot or Screencast

Screenshot 2023-02-13 at 11 28 24 AM
ChristopherChudzicki commented 1 year ago

tldr

This is happening because:


All image_src urls are full URLs with protocol, hostname, etc, except for OCW courses:

type:                   image_src full url?     example resource
----------------------------------------------------------------------
video                   YES                     https://open.mit.edu/api/v0/videos/7985/
podcast                 YES                     https://open.mit.edu/api/v0/podcasts/3/
course  mitx            YES                     https://open.mit.edu/api/v0/courses/5045/
course  bootcamps       YES                     https://open.mit.edu/api/v0/courses/12437/
course  ocw             NO                      https://open.mit.edu/api/v0/courses/9024/

OCW urls are treated specially: the database url stores a relative url (path-absolute, but no protocol or hostname), for example /courses/8-13-14-experimental-physics-i-ii-junior-lab-fall-2016-spring-2017/d46df724033fc5b0de90180e229aad8b_8-13-14f16-s17.jpg. The frontend then detects whether a resource is from OCW, and, if it is, prepends OCW_NEXT_BASE_URL.

Except those frontend shenanigans don't work when a learning list (which does not have platform=ocw) has an image_src coming from an ocw course.

Suggestion: We should use full urls for OCW image_src resources. (and for OCW resource.url values while we're at it).

mbertrand commented 1 year ago

Makes sense, might be as easy as changing this line to something like:

                "image": {
                    "src": urljoin(settings.OCW_NEXT_BASE_URL, course_json.get("image_src")),
                }

assuming course_json.get("image_src") is not null.

mbertrand commented 1 year ago

There might be some frontend shenanigans on the ocw search page too, will need to check for that. OCW search queries the open-discussions instance of Elasticsearch.

JenniWhitman commented 1 year ago

So after... much investigation and getting API working. I believe the solution requires another issue in OCW once we confirm that search receives an incomplete URL as well (will work on this now to confirm how it's populated there). Will need to update and deploy a conditional for those places where that url is pre-pended which checks if it's whole or partial before pre-pending anything so we don't end up with double base_url. Then we can implement a PR with the change to teh API.

JenniWhitman commented 1 year ago

Just to document it: after talking to Carey, it's likely we won't need to make any changes to OCW - I'm installing OCW adn testing, but it looks like the API and OCW front end are independent of one another. Once I verify this, we can wrap up with just the open-discussion changes (hopefully).

JenniWhitman commented 1 year ago

So after fully investigating - this WOULD change the URL as it's stored in the database which would impact the search results on OCW search. I think we'd have to either: add a new serializer to change the outgoing format only to this endpoint for course_catalog OR make the change prescribed here and change how OCW ingest those search results from open. I think the later would be the most uniform change and the least invasive overall especially for forward use.