-
Notifications
You must be signed in to change notification settings - Fork 214
Feedback utility functions #5088
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: unstable
Are you sure you want to change the base?
Feedback utility functions #5088
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.
Logically, everything in the code makes sense! Just a few small tweaks and some debugging is needed to resolve the UI issues encountered during testing. Additionally, I have a few thoughts on potential optimizations we could explore further.
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Show resolved
Hide resolved
feedback_type: FeedbackTypeOptions.ignored, | ||
feedback_reason: '', | ||
}); | ||
sendRequest(ignoredEvent); |
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.
It seems that we are currently sending a separate RecommendationsInteractionFeedback event for each "ignored" node. It might be better to send a single object that contains all the ignored node information, or at least consider batching or debouncing these requests.
Even if a user imports just 2 or 3 nodes on average, we end up generating a large number of these events, since other nodes are marked as ignored for each session.
This may not be something that needs to be addressed in this PR, as it could require some backend changes as well. I'm just thinking out loud here about potential optimizations we could explore.
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.
If I understand the feedback correctly, I think sending one HTTP request to create events for each node does seem ideal. The DRF should be able to handle that on the backend with a few tweaks, and the frontend may just need to be able handle an array of event objects. Alternatively, one by one can still work but we should await each request so we don't overload the server, and also ensure that that frontend code doesn't hold up the UX. My suggestion would be to go with the latter route and we can followup in a new PR to do the bulk request.
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.
I agree. the await should suffice for now. @ozer550, I can create a follow-up issue if you haven't already?
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.
yes this works! Thanks @akolson.
...tion/contentcuration/frontend/RecommendedResourceCard/components/RecommendedResourceCard.vue
Show resolved
Hide resolved
feedback_type: FeedbackTypeOptions.ignored, | ||
feedback_reason: '', | ||
}); | ||
sendRequest(ignoredEvent); |
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.
If I understand the feedback correctly, I think sending one HTTP request to create events for each node does seem ideal. The DRF should be able to handle that on the backend with a few tweaks, and the frontend may just need to be able handle an array of event objects. Alternatively, one by one can still work but we should await each request so we don't overload the server, and also ensure that that frontend code doesn't hold up the UX. My suggestion would be to go with the latter route and we can followup in a new PR to do the bulk request.
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Show resolved
Hide resolved
feedback_reason: this.recommendationsFeedback, | ||
}); | ||
if (this.validateFeedbackForm()) { | ||
sendRequest(rejectedEvent, 'patch') |
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.
@ozer550 Qn: would we still need to do a patch for this?
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.
Ahh I just saw that this is updating an already existing request! My apologies. I missed the patch part. I think this should be good. I was curious if there are any reasons to do this in a two step process instead of just creating one request at the end of the whole rejection event?
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.
We've implemented this as a two-step process because of the user experience (UX) design. Users first click the "mark not relevant" icon button on a card. This is followed by an optional request for additional rejection feedback. If this were a single-step process, we'd lose data for those who don't provide additional feedback. Also the initial confirmation snackbar disappears after a few seconds of inactivity, which could dismiss the event.
Happy to hear any other thoughts on how we can implement this.
Summary
This pr does the following;
References
#5057
#5058
Reviewer guidance
RecommendationsEvent
andRecommendationsInteractionEvent
models