-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: added NewAssignmentNotification - to all groupmembers #3312
feat: added NewAssignmentNotification - to all groupmembers #3312
Conversation
@vedant-jain03 kindly review. |
8ab7c11
to
e1ff1fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @VaibhavUpreti , here are my reviews!
Kindly go through and let me know if you have any doubt!
cc @tachyons
252a698
to
33907d4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
@VaibhavUpreti could you please add Also test if the assignment got deleted, the associated notification are destroying or not! I think |
Yeah Sure! I will write the required specs for these... And nothing seems to be broken after deleting the assignments ... The notifications are also getting cleaned up. |
@vedant-jain03 I have written the tests ... can u please check .. also I had sent u dm on slack regarding testing controller |
@VaibhavUpreti Try adding under the context when primary mentor is logged in, try this:
|
@vedant-jain03 I think this one is good to merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VaibhavUpreti Great work, thanks!
cc @tachyons, we can merge this!
@vedant-jain03 , @tachyons ... Anything else needed with this patch? or are we good to merge this |
An error occurred when fetching issues. View more on Code Climate. |
@tachyons @vedant-jain03 I haven't used the function "notification_type" as I think routing is part of ActionController and not present in custom classes. So the current code handles routing in the controller itself and "notify" function works for now. Is there any way we can use "redirect_to" method in our service class ... is so then we can keep this function else remove this. |
@tachyons made the controller "skinny" ... moved all the logic to the service class. Kindly review. |
project = notification.params[:project] | ||
redirect_to user_project_path(project.author, project) | ||
answer = NotifyUser.new(params).call | ||
return redirect_to group_assignment_path(answer.first_param, answer.second) if answer.type == "new_assignment" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why introduce a new
type
in service when we can already compare with schematype
. - Stick to
if-else
block as we have some other notification coming up which may have a different path other thanproject
orgroup
.
Else LGTM @tachyons WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #3294
Describe the changes you have made in this PR -
Screenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.