-
Notifications
You must be signed in to change notification settings - Fork 2
Search backend inject #31
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felipemontoya, thank you for preparing this! This is the correct repo for the draft.
| 'overview': lp.description or '', | ||
| 'subtitle': lp.subtitle or '', | ||
| 'language': 'en', # Default language | ||
| 'start_date': lp.created.isoformat() if lp.created else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: currently, the start_date is calculated on the frontend by taking the min start_date from all LP steps (courses). To support the presented use case, we should add start_date to the model (similarly to how we have the due_date. However, this will need an optimization to retrieve this information once for all LP steps, as retrieving it individually would be expensive. We may also consider introducing course-agnostic start dates for LPs in the future, so this is just a general note.
|
|
||
| # Get learning paths visible to user and filter by query | ||
| current_request = get_current_request() | ||
| learning_paths = LearningPath.objects.get_paths_visible_to_user(current_request.user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felipemontoya, doesn't this method cache the results for all users? I see that the search_results list does not store the user identifier anywhere. How does the access control work in the catalog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this is just trying to connect the results of LearningPath.objects.get... to the same API that is read from the /courses page. I did not see any cache decorators so I was not worried about caching, but I also did not test this with an anonymous user either.
For a proper implementation we need to check the permissions thoroughly and also a a better serializer below.
Proof of concept to bridge the learning paths (OC) and catalog app (RG) with minimal invasion to the openedx core.
This is just a POC, I'll be presenting this to the Core Product working group.
As I don't have any serious expectation that it will ever be merged I'll leave it as a draft.
The accompanying diagram: https://www.figma.com/board/XQRxoEDwRX2qcVFU4t6Qrr/Catalog-Extensibility---Implementation-timeline?node-id=27-52&t=501mbBkPdtoEZmiZ-1
@Agrendalath if this is noisy and you would rather not have it here, please let me know and I'll move it elsewhere.