Skip to content

Conversation

@flacial
Copy link
Member

@flacial flacial commented May 19, 2023

Overview

Currently, when a student receives a comment from a reviewer on their submission and attempts to reply to it, the reviewer doesn't get a notification. This behavior introduces difficulty in communication between the student and the reviewer, as the reviewer's only way to find out about the new comments by the student is to frequently visit the website. It can quickly degrade productivity for both the student who is trying to learn and the reviewers who are trying to teach.

Solution

Send a notification to all the people who wrote a comment on a certain submission's line except the comment author.

Missing requirements

comment section: comment(s) under a certain line in a submission

There are some things this PR does not introduce that are crucial for having an ideal notification system.

  1. Participants in a comment section can't opt out of receiving notifications about it.
  2. Participants cannot reply to or mention a user in a specific comment.

These shall be addressed in the project of adding a notification system (I could not find the issue for this project, but it did exist). @SlyBouhafs, any references?

Changes

  • Export the logic for sending a notification to a function in the addComment resolver.
  • Update the logic of sending a notification to send multiple ones by querying all the users who comment on a certain line or submission and sending a notification to them as long as they have a Discord ID.
  • Update the message. Discord embedding has two versions. One is for the reviewers, and the other is for the student.

Unrelated changes

These changes are not related to this PR, but they were required in order for the CI/CD to pass

  • Fix the flaky test in the exercises/[lessonSlug].tsx page by waiting for the router to be ready

Ideal solution

I believe this is not the ideal solution because sending simultaneous requests to our Discord bot server could overload the server and, by extension, slow down the server for other users. As of now, we don't have a lot of active users, and the comment section is almost always 1:1.

Related PR

flacial added 2 commits May 19, 2023 19:20
Reviewers will now get notified when a new comment is added to a line they previously commented on

Closes: 2849
@vercel
Copy link

vercel bot commented May 19, 2023

@flacial is attempting to deploy a commit to the c0d3-prod Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented May 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
c0d3-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2023 8:05pm

@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #2997 (c59d9fa) into master (df4ba19) will not change coverage.
The diff coverage is 100.00%.

❗ Current head c59d9fa differs from pull request most recent head 942fa49. Consider uploading reports for the commit 942fa49 to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2997   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          191       191           
  Lines         3582      3606   +24     
  Branches       972       979    +7     
=========================================
+ Hits          3582      3606   +24     
Impacted Files Coverage Δ
graphql/resolvers/addComment.ts 100.00% <100.00%> (ø)

@flacial flacial requested a review from JasirZaeem May 20, 2023 14:53
flacial added 2 commits May 21, 2023 23:56
This commit is unrelated to the current PR, but it's required for the CI to pass and the local pre-push hook to pass
@flacial flacial added this pull request to the merge queue May 21, 2023
Merged via the queue into garageScript:master with commit 72c8090 May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants