overhangio / tutor-discovery

Course Discovery plugin for Tutor
GNU Affero General Public License v3.0
11 stars 40 forks source link

feat: upgrade to nutmeg #31

Closed regisb closed 2 years ago

ghassanmas commented 2 years ago

Thats not really a review per say I just wanted to make sure we are all on the same page before upgrading the forum plugin to v1 :wink:

ghassanmas commented 2 years ago

@regisb this is same branch that is used for nutmeg demo, right? If yes could you add this settings override in the plugin, I think openedx-lms-common-settings is the right place. SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING = False

https://github.com/openedx/edx-platform/blob/b4fecd620bf3bb4ab32ccbcf475cfac6f9fb7814/lms/envs/common.py#L4093

The reason I am asking for this, is that I think we change it to False, then course_visbility, in course advace settings will be reflected /courses hence: https://github.com/openedx/edx-platform/blob/a27247d14d2b77234fd004988f96258e29d0d000/lms/lib/courseware_search/lms_filter_generator.py#L44-L61

But anyway, may be you don't agree that this the right place to chagne this settings? IMO the SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING should default to False in the first place.

regisb commented 2 years ago

Fantastic catch @ghassanmas! Setting SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING = False in the LMS common settings does allow us to hide the course! I will push this setting to be the new default in Nutmeg. It will be present in the default installation, not in the tutor-discovery plugin: https://github.com/overhangio/tutor/pull/635/files#diff-c7b8fd3e439b9c293fbec7493376cee7fbccf4c641b113b77d68167f9585d07a

EDIT: no it didn't work... EDIT 2 : it did work!

regisb commented 2 years ago

Scratch that @ghassanmas, defining SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING = False does not actually hide the courses with no visibility. I have no idea why...

ghassanmas commented 2 years ago

@regisb The way it worked and test it as: I added exactly in lms/production.py SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING = False. And hided the courses in the /courses page only, becaeuse this settings is actually not for the discovery service, its for discovery featuer, which is responsible for displaying the courses at /courses. Also it works only if visibility is none not if its about. And last caveate is that sometimes course index doesn't have a field about "catalog_visbility" and the way to force that field to appear in course index is 1) to trigger advnace settings save and 2) reindex the course. I am not really 100% sure about those two steps, but I am sure that "catalog_visibility" should be in course index.

ghassanmas commented 2 years ago

To check if catalog_visbility fieled exists, you can go /courses page and check the response of the XHIR request /course_discovery here is a screenshot for nutmeg demo

image
regisb commented 2 years ago

Ok @ghassanmas I have no idea what's going on but it appears that this setting works. Thanks a lot for your research!