Skip to content
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

Backend integration code for flagging priority questions for faculty issue #12 + NPM test code coverage fix #13

Merged
merged 2 commits into from
Feb 28, 2025

Conversation

tjpatel04
Copy link
Contributor

Deleted notifications.js initial changes feature from sprint 1 to fix code coverage and testing to successfully run npm test. Decided to focus on different feature implementation.

Changed topics/create.js, edit.js, posts/create.js, and added priorityflag.js file to implement the backend feature code for issue #12. Feature flags any post with certain priority and assigns a color (green,orange,red) with red being the highest priority to a students post for increased faculty visibility.

Copy link
Contributor

@Sharkesh111 Sharkesh111 left a comment

Choose a reason for hiding this comment

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

I left a comment about the functionality of how we assign priority levels to a comment that has multiple priority levels in it. This is just a small edge case to consider, but other than that your code looks great!

Copy link
Contributor

Choose a reason for hiding this comment

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

With this code, the way you are looping through the priority map from low to high, it makes it so that in the edge case where the user enters high priority and low priority in the same comment, the comment will be flagged as low priority due to the order of the loop.

We may want the comment to be flagged as the highest priority mentioned in the comment.

In this case, just reverse the order you initialize PRIORITY_MAP from ascending order by priority level to descending order.

@Caryzxy
Copy link
Contributor

Caryzxy commented Feb 27, 2025

This looks good, should have no conficts with other functions in other PR

@Caryzxy Caryzxy merged commit 7ad8e24 into main Feb 28, 2025
0 of 2 checks passed
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.

3 participants