-
Notifications
You must be signed in to change notification settings - Fork 0
feat: send status notifications #109
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
base: master
Are you sure you want to change the base?
Conversation
fny
left a comment
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.
Did realize these stay as pending
app/workers/status_notify_worker.rb
Outdated
| @@ -0,0 +1,21 @@ | |||
| # frozen_string_literal: true | |||
| class StatusNotifyWorker < ApplicationWorker | |||
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.
We need a better name for this and NotifyWorker. It's really confusing right now.
Maybe StatusAlertAdminsWorkers and StatusAlertUserWorker?
app/workers/status_notify_worker.rb
Outdated
| users_to_notify = Set.new | ||
|
|
||
| locations.map(&:location_accounts).flatten.each do |la| | ||
| if la.role.admin? && la.user.id != notify_except |
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.
What does != notify_except do?
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.
@fny It excludes the user who submitted the status. I thought he/she wouldn't have to be notified about the submission he/she made. Let me know if I was wrong. And I see that the submitter can be different from the person of the symptom - sometimes even an administrator.
spec/requests/symptom_survey_spec.rb
Outdated
| expect(response_json).to have_key(:data) | ||
| expect(response_json[:data][:attributes]).to include(status: 'pending') | ||
| expect(user.last_greenlight_status.status).to eq(GreenlightStatus::PENDING) | ||
| expect_work(StatusNotifyWorker) |
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.
Can we also check that the proper users have received an email?
app/workers/notify_worker.rb
Outdated
| @@ -0,0 +1,39 @@ | |||
| # frozen_string_literal: true | |||
| class NotifyWorker < ApplicationWorker | |||
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.
Need a better name
app/workers/notify_worker.rb
Outdated
| def perform(user_id, symptom_holder_id, status) | ||
| user = User.find(user_id) | ||
| symptom_holder = User.find(symptom_holder_id) | ||
| status_color = status == 'pending' ? 'Yellow' : 'Red' |
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.
Make this an I18n translation for all the following. I've added the spanish
"GreenlightStatus.absent": "Ausente",
"GreenlightStatus.cleared": "Aprobado",
"GreenlightStatus.not_submitted": "No Enviado",
"GreenlightStatus.pending": "Pendiente",
"GreenlightStatus.recovery": "Recuperación",
"GreenlightStatus.cleared_color": "Aprobado",
"GreenlightStatus.not_submitted_color": "No Enviado",
"GreenlightStatus.pending_color": "Pendiente",
"GreenlightStatus.recovery": "Recuperación",
The ouptut format should be "type (color)" for example "Recovery (Red)".
GreenlightStatus.red 'rojo'
GreenlightStatus.yellow 'amarillo'
GreenlightStatus.green 'verde'
fny
left a comment
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.
I'm going to merge this into the next release.
No description provided.