Add Endpoints for Notifications #950
Conversation
- Create Notifications module and NotificationType model - Start migration for adding notifications - No functionalities added yet only template code
notification entry
Changes: * Replaced is_enable to is_enabled for Notification Preferences * Update default is_enabled to true for Notifcation Types
`import_config` must always appear at the bottom for environment specific configurations to be applied correctly. All configurations after this line will overwrite configurations that exists in the environment specific ones.
- remove auto-generated controllers and views that are not used
If user preference has no time option, use the time_option from notification_config instead. This is so that the behaviour of these users with no preferences would always follow the default chosen by the course admin
* Query returns an array of preferences per config due to LEFT OUTER JOIN, the change ensures either nil or the first preference is returned * It is guaranteed there is maximally one preference in the array due to the unique constraint in the new migration
- Add put upsert time options endpoints - Fix changeset issues with upsert noti config endpoints
* Removed extra unused route
There was a problem hiding this comment.
Thanks for the quick work! However, I think the design needs major changes.
First of all, try to follow the current routing scopes, if a notification belongs to a course, semantically it should be under /course. In that way, you will naturally pipe through authentication checks, course checks sand staff checks.
Second, please separate admin routes from student routes (or do you want to separate them from staff also?). Do think about the privilege of each route. The backend needs to be intact without the assumption of the well-behaviour of the frontend.
Lastly, please clean up unnecessary comments and use query styles in the piping fashion(which aligns with the current code base). Try to use Logger instead of IO for logging.
| @@ -169,6 +169,19 @@ defmodule CadetWeb.Router do | |||
| ) | |||
| end | |||
|
|
|||
| # Notifications endpoints | |||
There was a problem hiding this comment.
How are these APIs being called from the frontend? Why are they not piped through authentication and course-specific assignment? I am not sure what might happen here. If some request is fired from a student that already has his log in session expired or if the student changed course(which might lead to some cache issue or privilege issue)...
There was a problem hiding this comment.
Also please separate the admin routes from the student routes
There was a problem hiding this comment.
This could be potentially very dangerous as some students may edit the configs that only admins can edit. We should not assume that the frontend will behave as expected.
There was a problem hiding this comment.
or is there any justification that these APIs do not need any authentication at all?
| if length(ungraded_submissions) < ungraded_threshold do | ||
| IO.puts("[AVENGER_BACKLOG] below threshold!") | ||
| else | ||
| IO.puts("[AVENGER_BACKLOG] SENDING_OUT") |
There was a problem hiding this comment.
is a logger better here?
| query = | ||
| from(n in Cadet.Notifications.NotificationConfig, | ||
| where: n.course_id == ^course_id | ||
| ) |
There was a problem hiding this comment.
Why is the query here in a different style?
| ntype.for_staff == ^is_staff and | ||
| n.course_id == ^cr.course_id and | ||
| (p.course_reg_id == ^cr.id or is_nil(p.course_reg_id)) | ||
| ) |
There was a problem hiding this comment.
A different style of query. Please stick to piping. |>
| # end | ||
|
|
||
| # case Repo.delete(time_option) do | ||
| # JUNYI AND SANTOSH: NOT DEFINED??? |
There was a problem hiding this comment.
please clean up unused comments as well.
Changes
Adding extra endpoints needed for notifications