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

Add priority to alert model, add sorting in display of Alerts page #77

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cyu6
Copy link
Collaborator

@cyu6 cyu6 commented Sep 3, 2020

Summary:

  1. Add "priority" as a property of the Alert model. To make this change, I also had to change many of the other mock / backend files where alerts are created, which is why there are many files changed in this PR.
  2. On the Alerts page, users can change priority of each alert, and also sort the alerts in the list by priority and by date. Here's a short GIF of that working:
    React App (1)
  3. On each individual alert's page, users can also change the priority of each alert. Here's what that looks like:
    Screenshot 2020-09-03 at 9 44 07 AM - Display 2
  4. To change the priority of alerts in the backend, I added a doPost method to the Alert Visualization servlet, and this method is called by both the AlertsContent and AlertInfo components in order to change priority of alerts.
  5. Tests are added to AlertVisualizationServlet and AlertInfo

@cyu6 cyu6 requested review from melodychn, paulaksp, hehhjiang and Leodson and removed request for melodychn and paulaksp September 3, 2020 13:54
@cyu6 cyu6 linked an issue Sep 3, 2020 that may be closed by this pull request
@cyu6 cyu6 requested a review from paulaksp September 3, 2020 16:44
* Read from the request body, which contains data in the format "alertId statusToChangeTo".
* For example, the body could be "4785074604081152 RESOLVED".
*/
private String[] processRequestBody(HttpServletRequest request)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why the data format is as what you mentioned (alertId statusToChangeTo) separated by the space?

Also as usual prefer structured as compared to unstructured data :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops I forgot to change this comment. I will change the comment
So yes you can pass an object to the backend, but I found it more difficult to deal with processing the object rather than just using this unstructured method.
In hindsight, I could have done this by passing in the data as parameters, because there are only two...

const { alert } = this.state;

// We have to get the P0, P1, or P2 version to match with enum representation in backend.
const numToEnum = Object.keys(priorityLevels)[Object.values(priorityLevels).indexOf(newPriority)];
Copy link
Collaborator

Choose a reason for hiding this comment

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

i was a bit confused on what is this numToEnum does. so you need to do the mapping with the map to convert it back?

seems the priority is just the "0", "1", "2" (the numerical value) etc for easier sorting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup! It's for easier sorting, but to translate it back to the backend (so that the backend understands 0, 1, 2) should convert it back. If I didn't do it in the frontend, I think I would have to do it in the backend anyway

const numToEnum = Object.keys(priorityLevels)[Object.values(priorityLevels).indexOf(newPriority)];
fetch('/api/v1/alert-visualization', {
method: 'POST',
body: alert.id + " " + numToEnum,
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh i think this is why the post servlet above (in the previous file I commented had the space in the body), could we not change this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refer to earlier comment - I will change this to passing through parameters in the rquest rather than the body

throws ServletException, IOException {
String[] data = processRequestBody(request);
Long id = Long.parseLong(data[0]);
String priority = data[1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

i thought it was "statusToChangeTo" and not priority

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

typo :'(

const DATE_DELIMITER = '/';

/** Format JavaScript date in the form MM/DD/YYYY if mFirst is true, otherwise YYYY/MM/DD. */
export function formatDate(date, mFirst) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

monthFirst (this is from the other PR, probably ude to some out-of-sync thing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it's out-of-sync i changed it to monthFirst in the other one

frontend/src/AlertsManagement/AlertsList.js Outdated Show resolved Hide resolved
@cyu6 cyu6 requested review from paulaksp and melodychn September 4, 2020 03:45
Copy link
Collaborator

@melodychn melodychn left a comment

Choose a reason for hiding this comment

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

Thanks for fixing all the small bugs :)

great job! you added a lot of great features <3

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.

Basic priority ranking and/or sorting options for alerts
4 participants