Refactor user feedback logic #2534
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Allows us to fetch and get recording MBIDs simply, with implementation details in the hook itself, and prevents hammering our API with a million calls by batching calls (throttling)
for playing_now listens, unmatched listens and others that might not have an MBID in the listen metadata
This is the feedback fetching and management logic that was duplicated for each entry page.
ES6 module interop blablablabla
… from parent components that were previously handling all the feedback logic.
Rewriting the tests currently, but it's basically rewriting from scratch and with extra issues due to going from a class component to a functional component (issues with the Enzyme library)
mayhem
approved these changes
Aug 8, 2023
React hooks should be pure, and this one wasn't. Instead, create a RecordingFeedbackManager class and store an instance in the global context Also adds a subscription system to update other ListenCard for the same MBID/MSID so that changes to one listencard will sync with other listencard for the same recording
Used in pretty much all our test files… Something else to refactor !
|
Tests are not passing. Something the enzyme library didn't like. Will investigate. |
instead of handling JSX and the toast library in the RecordingFeedbackManager which shouldn't need to know about any of those details (on top of creating issues with Enzyme tests…)
Enzyme really doesn't like functional components, and the whole test suite was breaking because it doesn't support it. Mocked the hook instead.
Not sure why the tests are failing in CI but passing locally…
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we first implemented feedback on Listen Cards, we only did so on one page.
As the ListenCard component was refactored from many different components into a single all-powerful ListenCard, the feedback logic has been copied over to every top-level page.
This meant each top-level page had logic for:
I've finally had enough of duplicating the code, and refactored it all into a custom react hook which is only used in one single component (ListenFeedbackComponent).
Each ListenCard, when instantiated, now manages its own feedback automatically using the recording MBID or MSID in its listen.
One of the reason this wasn't done before is that I wanted to avoid pelting our API with 50 separate requests for each page of listens.
Now the useFeedbackMap hook handles fetching feedback in batches (throttled) as well as saving the feedback in a singleton Map object to avoid more API calls.
Note: I completely removed ListenFeedbackComponent.test.tsx test file. I am hitting some issues with Enzyme again which doesn't handle React functional components. I'm working on the tests but they'll probably come in a separate PR as we might need to start running two separate test suites in parallel…