Filter 'published' the same for /learningpaths and /learning_resource?resource_type=learning_path

[ChristopherChudzicki]

What are the relevant tickets?

A potential fix for #67

What's this PR do?

This PR refactors how the published filtering works so that /learningpaths and /learning_resources?resource_type=learning_path return the same results.

How should this be manually tested?

1. View http://localhost:8063/api/v1/learningpaths/ and http://localhost:8063/api/v1/learning_resources/?resource_type=learning_path as anonymous user or a user without learning_path_editor permission (or as admin).
   • both endpoints should not include unpublished learningpaths
2. Repeat (1) but now with the learning_path_editor permission (or as admin).
   • both endpoints should include unpublished learningpaths

[codecov]

Codecov Report

Merging #71 (7468cb1) into main (eb5a550) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #71   +/-   ##
=======================================
  Coverage   84.17%   84.18%           
=======================================
  Files         401      401           
  Lines       16968    16969    +1     
  Branches     2707     2707           
=======================================
+ Hits        14283    14285    +2     
  Misses       2368     2368           
+ Partials      317      316    -1     

Files Changed	Coverage Δ
learning_resources/views.py	97.89% <100.00%> (+0.02%)	⬆️

... and 6 files with indirect coverage changes

[mbertrand]

I originally thought of this read-only viewset as intended just for browsing published resources, and if one needed to do any CRUD work, a different endpoint would be used (only LearningPath fits that need right now). I'm fine with discarding that assumption, but one potential issue with this change is that editors/admins will always see both published and unpublished results here for learning paths, and there may be times when it is only appropriate to show published paths (for instance, a carousel of top 10 newest learning paths or top 10 newest resources in general). One way to avoid this is to add published to the filterset_fields above, and filter by ?published=true whenever necessary. Can you add that in?

[mbertrand]

PS One potential drawback to this though is that using ?published=False would allow any users to see unpublished courses, programs, videos, paths, etc - which may or may not be desirable? So might need to somehow force published=True for anyone except admins (and editors for learningpaths only)

[mbertrand]

Maybe add an api/v1/learning_resources/published/ endpoint to be used by carousels and such? And leave the code above as is (editors/admins always see published + unpublished paths unless using the /published/ endpoint).

[Ferdi]

@ChristopherChudzicki can you summarize what the changes are ?

[ChristopherChudzicki]

@Ferdi My goal with this change was to make it so that if a user has permission view a learningpath, then it is visible at /learning_resources/:id or /learnpaths:id/. Currently, /learning_resources/ will not return unpublished learningpaths, whereas /learningpaths/ will.

@mbertrand

I originally thought of this read-only viewset as intended just for browsing published resources, and if one needed to do any CRUD work, a different endpoint would be used (only LearningPath fits that need right now).

I was not thinking to use the /learning_resource/ endpoints for editing, but for populating a list of things that might be edited: It does seem easier to me when fetching details or listings to use the single API /learning_resources/:

For example: the frontend caches API responses. If we sometimes request LearningPaths from /learning_resources/:id, and sometimes from /learningpaths/:id, then editing a LearningPath requires two cache invalidations instead of one. This isn't a big deal. Two isn't much more complicated.

@mbertrand Re

there may be times when it is only appropriate to show published paths (for instance, a carousel of top 10 newest learning paths or top 10 newest resources in general). One way to avoid this is to add published to the filterset_fields above, and filter by ?published=true whenever necessary. Can you add that in?

Good point. If we do change this API, then rather than explicitly saying "published only", maybe published=true should be the default, and we could filter to explicitly allow unpublished.

[ChristopherChudzicki]

@mbertrand I'm going to close this for now, but I'll leave the issue open:

For the learningpaths work, /learningpaths/:id has an advantage over learning_resources/:id: the former will 404 if the id is not a learningpath, which allows the frontend to redirect naturally if /frontend_page/learningspaths/123 is actually a course.

You raise a good point point about published filtering; I left a note about that in #67
Continuing to use /learningpaths for the fetching seems just as easy