search

Comp-490-SeniorProject / site

MIT License
0 stars 1 forks source link

Build component for notifications page #35

Open mndzamel opened 2 years ago

mndzamel commented 2 years ago

Build the notifications page component that shows the notification settings for each test.

2 parts:

  1. Implement the notifications page as an Angular component. Header and sidebar are separate components. Notifications only for tests (automatic and manual) with sensors. Bootstrap is used to improve layout and styling.
  2. Connect the notifications page component to the backend API using Typescript. Notifications may also be configured from the page. Model: Notification settings
MarkKoz commented 2 years ago

The model for a notification is here, so you can see which fields it needs. https://github.com/Comp-490-SeniorProject/site/blob/main/web/api/models/notification_settings.py#L22-L38

The front end for this essentially needs to be a form. The user should be able to select any Test for any Device, and then create a new notification. A Test can have multiple notifications added for it.

Once they select a Test, they need to write a message, which just any string that they want the notification to display when the notification is sent. They also need to select a destination, which can be either web notifications or email notifications. For now, assume that email is always a valid choice. In the future, we will need to check that the user actually provided an email when they signed up.

Finally, the most complex part is the condition. This is an expression which determines if the notification should be sent. The expression language is relatively powerful, probably more powerful than we need. All the operations are documented here https://jsonlogic.com/operations.html

I think for now, you can make it a very simple implementation that just accepts the JSON directly from the user. In the future, if we have time, we can improve the UI by added some kind of expression builder.

Once all that data is filled by the user, you can send a POST request to the /api/notification_settings endpoint (it's an authenticated endpoint). It should return 200 for success. If there is an authentication issue then I think it's either 401 or 403. If data is invalid then probably just 400. And of course 5xx error for internal errors may also occur.

alinaah commented 2 years ago

Please list down the form fields along with the expected values for them for this notifications page?

MarkKoz commented 2 years ago

My previous comment already lists all fields and explains their values. I even linked to the model.

alinaah commented 2 years ago

Notification form fields: Test: A dropdown having tests as options Condition: (I assume it will be a text field), will take json expression as input Message: A text field, The message to display in the notification. Destination: Checkboxes, to select where to send the notification, by default will be WEB.

SO this is what I have concluded

Now my question is about the condition field

  1. What will be the input type?
  2. What would be the expected input?
  3. Do we have to send the input as it is to the back end or we have to do some processing at the front end?
MarkKoz commented 2 years ago

What will be the input type?

JSON is text, so it is a text field.

What would be the expected input?

Text that is valid JSON. More specifically, it has to be valid JsonLogic.

Do we have to send the input as it is to the back end or we have to do some processing at the front end?

Send it as-is. The backend will validate the input and return (I think) HTTP status code 400 if the condition is invalid.

alinaah commented 2 years ago

Ok, so here is the final list or all the requirements, let me know if I missed anything:

Notifications form:

  1. Test: A dropdown having tests as options
  2. Condition: (I assume it will be a text field), will take json expression as input
  3. Message: A text field, The message to display in the notification.
  4. Destination: Checkboxes, to select where to send the notification, by default will be WEB.

Send the data to back end the display success/error message according to the response receivrd.

MarkKoz commented 2 years ago

You should add a dropdown to list the devices so that the user can select a device first. The tests shown in the tests dropdown should be tests for the device selected. This just helps the user filter out the tests.

alinaah commented 2 years ago

Q1. I cannot display tests in Notification settings dropdown without changing backend. What should I do in this situation?

Q2. Notification settings create API is not there in backend. So for now I just can put notification settings component without integrating it. Is it okey..?

MarkKoz commented 2 years ago

@alinaah

  1. I mistakenly thought filtering was already supported, but it wasn't. I opened a PR to add support for it. It also includes a description of how to use the filters. https://github.com/Comp-490-SeniorProject/site/pull/60

  2. I'm not sure what you mean. The endpoint exists and is routed. It should be a POST requests to /api/notification_settings/. Is it not working for you?

alinaah commented 2 years ago

@MarkKoz

  1. I tested personally there is no POST request working for notification_settings. You can check the notification_settings viewset. Only GET request function is there.
MarkKoz commented 2 years ago

@MarkKoz 2. I tested personally there is no POST request working for notification_settings. You can check the notification_settings viewset. Only GET request function is there.

That's not correct. It does support POST. However, there was a bug that was always causing a JSON validation error for the condition field. Is that what you meant? If not, then I don't know what you're referring to.

Anyway, I opened a PR to fix the bug I mentioned, and it includes an example of how a POST request should be made to that endpoint https://github.com/Comp-490-SeniorProject/site/pull/61