Skip to content

Refactor user lists and learning paths hooks out of learning resources #1842

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

Merged
merged 5 commits into from
Nov 21, 2024

Conversation

jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Nov 21, 2024

What are the relevant tickets?

N/A

Description (What does it do?)

Refactors the learning resource API hooks to pull out Learning Paths and User lists. This align the cache key structure with the API path structure / data models, keying ["learningPaths", ...] and ["userLists", ...] at the top level. Much of the data on the site relates to learning resources, so the learning resources API handling was getting large with relationships as children of learning resources.

This identified a lot of the user list code as unused. It is commented out in this PR so we can first determine whether it is worth keeping for any upcoming usage.

How can this be tested?

Non-functional change. Test should pass and everything should work as previously, with particular attention to user list and learning path memberships and their action buttons on resource cards and list items; ensuring that these behave as expected as we add and remove from lists and navigate between screens (homepage carousels, search results, dashboard user list details and admin learning paths).

Additional Context

Refactored to lay things out more clearly for https://github.com/mitodl/hq/issues/6032, which introduces memberships that specify the relationships between learning resources and lists (the membership lists are on the learning path and user lists APIs).

@jonkafton jonkafton force-pushed the jk/refactor-learning-resources-api branch from 1478d59 to 1b6a124 Compare November 21, 2024 08:47
@mbertrand mbertrand added Needs Review An open Pull Request that is ready for review and removed Needs Review An open Pull Request that is ready for review labels Nov 21, 2024
@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Nov 21, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Could you remove the commented out stuff and delete the empty test before merging?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mentioning: Most of this file should go away as part of https://github.com/mitodl/hq/issues/6032

thank goodness.

// })
// }

// const useUserListRelationshipCreate = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioning: This hook, and associated learning path ones, were used prior to #1463. Prior to 1463, items ("relationships") were created when user checked or unchecked the checkbox. Now they are all added at once when user clicks "Save".

@jonkafton jonkafton merged commit ae68125 into main Nov 21, 2024
11 checks passed
@jonkafton jonkafton deleted the jk/refactor-learning-resources-api branch November 21, 2024 15:52
mbertrand pushed a commit that referenced this pull request Nov 22, 2024
#1842)

* Refactor learning path API hooks out of learning resources

* Refactor user list API hooks out of learning resources. Dead code commented out

* Move post mutation helpers to hooks files

* Remove empty file

* Remove commented code
@odlbot odlbot mentioned this pull request Nov 25, 2024
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants