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.

Sign up for GitHub

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

Jump to bottom

TC_AUTHOR_133: STUDIO: SETTINGS: ADVANCED: Course visibility in catalog #164

Closed
DeanJayMathew opened this issue Apr 27, 2022 · 8 comments
Closed

TC_AUTHOR_133: STUDIO: SETTINGS: ADVANCED: Course visibility in catalog #164

DeanJayMathew opened this issue Apr 27, 2022 · 8 comments
Assignees
@ghassanmas
Milestone
Nutmeg.1

Comments

@DeanJayMathew
Copy link

Steps to reproduce:

Go to https://studio.nutmeg.demo.overhang.io/home with staff credentials
Go to Advanced Settings: Course Visibility in Catalog
Set to "none":

Screen Shot 2022-04-27 at 12 57 08

Expected result:
Go to https://nutmeg.demo.overhang.io and the course should NOT be visible.
Go to https://nutmeg.demo.overhang.io/courses and the course should NOT be visible.

Actual result:
Course is NOT visible on https://nutmeg.demo.overhang.io <-- this is good
Course is still visible on https://nutmeg.demo.overhang.io/courses <-- this is the bug

Screen Shot 2022-04-27 at 12 56 58
@DeanJayMathew
Copy link
Author

label: testing nutmeg

@BbrSofiane BbrSofiane added this to the Nutmeg Release milestone May 14, 2022
@ghassanmas
Copy link
Member

ghassanmas commented May 15, 2022 •
edited
Loading

I was trying to debug this, and I think simliar issue was raised in tutor forum before1. However in the forum thread, it was resolved by disabeing the discovery service, where in nutmeg demo discovery is enabled. And I guess one should assume that using discovery service shouldn't stop course author/admin from being able to hide the course

However I wasn't sure if the "Course visibility in catalog" affects discovery service? I couldn't find any documentation about that explicitly. @regisb do you have any though on this?

Footnotes

  1. https://discuss.overhang.io/t/catalog-visibility-in-tutor-deployed-open-edx/214 ↩

@ghassanmas
Copy link
Member

ghassanmas commented May 15, 2022 •
edited
Loading

Also to confirm what @DeanJayMathew have repoted:

Course is NOT visible on https://nutmeg.demo.overhang.io/ <-- this is good
Course is still visible on https://nutmeg.demo.overhang.io/courses <-- this is the bug

The first case it worked becasuse the result wasn't retrived by discovery/search service while second failed because it gets from discovery service

@DeanJayMathew
Copy link
Author

@ghassanmas

Thank you for debugging this, well noted.

The Discovery service is an important service we use in almost all our separate instances. So for me it's a high priority. I will speak to my colleagues for their input too since they are experienced in this discovery service.

Let's aim to resolve this ticket this week.

ghassanmas added a commit to ghassanmas/edx-search that referenced this issue May 16, 2022
@ghassanmas
fix: courses are visible /courses when shouldn't
c61178a 
  When discovery service is enabled, then the /courses page in
  LMS will not be filled with courses **initially**, so that a
  frontend script will send XHR request to fetch courses using
  POST /search/course_discovery/, the LMS then will proxy the
  request to edx-search.

  The probelm is when a course is hidden using advance setting,
  in studio, the edx-search is not affected or is not concern with
  that setting.

  So here I am adding an edge case, where if in the search
  request the 'catalog_visibility' is not set, it will default
  to 'both' meaning, by default it will *only* return courses
  that are suppose to be visible in the catalog.

  This fixes openedx/wg-build-test-release/issues/164.
ghassanmas added a commit to ghassanmas/edx-search that referenced this issue May 16, 2022
@ghassanmas
fix: courses are visible /courses when shouldn't
d4844d6 
  When discovery service is enabled, then the /courses page in
  LMS will not be filled with courses **initially**, so that a
  frontend script will send XHR request to fetch courses using
  POST /search/course_discovery/, the LMS then will proxy the
  request to edx-search.

  The probelm is when a course is hidden using advance setting,
  in studio, the edx-search is not affected or is not concern with
  that setting.

  So here I am adding an edge case, where if in the search
  request the 'catalog_visibility' is not set, it will default
  to 'both' meaning, by default it will *only* return courses
  that are suppose to be visible in the catalog.

  This fixes openedx/wg-build-test-release/issues/164.
@ghassanmas ghassanmas mentioned this issue May 16, 2022
Closed
ghassanmas added a commit to ghassanmas/nutmeg-demo that referenced this issue May 16, 2022
@ghassanmas
fix: course visibility
3703c5f 
  This adds a plugin to override edx-search in order to fix
  openedx/wg-build-test-release/issues/164 also for more
  context the upstream PR openedx/edx-search/pull/124
@ghassanmas ghassanmas mentioned this issue May 16, 2022
Closed
ghassanmas added a commit to ghassanmas/nutmeg-demo that referenced this issue May 16, 2022
@ghassanmas
fix: course visibility
f70f55d 
  This adds a plugin to override edx-search in order to fix
  openedx/wg-build-test-release/issues/164 also for more
  context the upstream PR openedx/edx-search/pull/124
ghassanmas added a commit to ghassanmas/nutmeg-demo that referenced this issue May 16, 2022
@ghassanmas
fix: course visibility
66562a1 
  This adds a plugin to override edx-search in order to fix
  openedx/wg-build-test-release/issues/164 also for more
  context the upstream PR openedx/edx-search/pull/124
@regisb
Copy link
Contributor

regisb commented May 16, 2022

@DeanJayMathew I'm not sure if this question can be resolved by next week, or even whether it's an actual issue at all. Let me refer to my answer to that forum topic : https://discuss.overhang.io/t/catalog-visibility-in-tutor-deployed-open-edx/214/6?u=regis

/courses is not the catalog, and the courses that are displayed there are unaffected by the visibility settings from the advanced settings. If you’d like to disable this page entirely, then you need to set FEATURES["ENABLE_COURSE_DISCOVERY"] = False in the lms settings.

In other words, the "course visibility in catalog" does NOT refer to the listing in the /courses page. It's very weird, but that's the way it has been since, well forever. If you ask me, I think that this advanced setting should indeed govern the course listing in /courses. We should simply be aware that this would not classify as a "bug fix" but a "breaking change feature".

@regisb regisb mentioned this issue May 16, 2022
Closed
ghassanmas added a commit to ghassanmas/edx-search that referenced this issue May 26, 2022
@ghassanmas
fix: courses are visible /courses when shouldn't
48c31b7 
  When discovery service is enabled, then the /courses page in
  LMS will not be filled with courses **initially**, so that a
  frontend script will send XHR request to fetch courses using
  POST /search/course_discovery/, the LMS then will proxy the
  request to edx-search.

  The probelm is when a course is hidden using advance setting,
  in studio, the edx-search is not affected or is not concern with
  that setting.

  So here I am adding an edge case, where if in the search
  request the 'catalog_visibility' is not set, in filter or
  or exculde it will default, to exculde 'none' and 'about'
  meaning only courses with 'catalog_visibility' set to 'both'
  will be returned or visbile in the /courses page

  This fixes openedx/wg-build-test-release/issues/164
ghassanmas added a commit to ghassanmas/tutor that referenced this issue Jun 7, 2022
@ghassanmas
chore: change default of catalog settings
77d01c3 
 This partialy fixes #openedx/wg-build-test-release/issues/164
 The flag would make it possible to hide courses in the /courses
 page when ENABLE_COURSE_DISCOVERY is true.
 For more context check the forum thread:
 https://discuss.openedx.org/t/request-for-feedbak-breaking-changes-for-course-visibility/7341
@ghassanmas ghassanmas mentioned this issue Jun 7, 2022
Closed
@NeOneSoft
Copy link

Hi @ghassanmas !!!! do we have any update regarding this issue? Thanks in advance

@ghassanmas
Copy link
Member

@NeOneSoft Yes I will comment below the summary of the finding. I will just re post what I already repoted in the forum the forum.

On the other hand, while I was going over testing for my changes, I find that edx-search has a setting for overriding filter class, and the LMS does override it and it use this filter instead edx-platform/lms_filter_generator.py at a27247d14d2b77234fd004988f96258e29d0d000 · openedx/edx-platform · GitHub . And filter shuold skip course visibility to none if SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING = False. I am not 100% sure about that, since I just found about it and didn’t test it yet. ref

Given the above finding, tutor was changed to use that flag by default.

Note using that flag, would enable to hide the course if the settings is "none" only. It won't work for "about".

Repository owner moved this from 🏗 In Progress to ✅ Done in Build-Test-Release Working Group Aug 8, 2022
@NeOneSoft
Copy link

Thank you so much @ghassanmas for your update here!!!

@mariajgrimaldi mariajgrimaldi mentioned this issue Dec 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ghassanmas ghassanmas

Labels
None yet
Projects
Build-Test-Release Working Group
Status: Done
Milestone
Nutmeg.1
Development

Successfully merging a pull request may close this issue.

fix: courses are visible /courses when shouldn't ghassanmas/edx-search
5 participants
@regisb @DeanJayMathew @BbrSofiane @ghassanmas @NeOneSoft