Question being answered notification #94
Conversation
…fications to on-duty staff only, allow notification enabled for student
…aining notification
| } else if (action.type === UPDATE_QUESTION.SUCCESS) { | ||
| const { question } = action | ||
| // If question is marked as being answered and it is the student who ask this question | ||
| if (question.beingAnswered && user.id === question.askedById) { |
There was a problem hiding this comment.
In the future, we might allow students or staff to edit questions while they're being answered. To be safe, I'd check for the question in the existing state and make sure that this UPDATE actually represents a transition from not being answered to being answered.
There was a problem hiding this comment.
Done, let me know if there is any other changes needed!
| const { question } = action | ||
| // If question is marked as being answered and it is the student who ask this question | ||
| if (question.beingAnswered && user.id === question.askedById) { | ||
| const title = 'Your question is being answered' |
There was a problem hiding this comment.
Maybe "STAFFNAME is answering your question" instead?
| import { baseUrl } from '../util' | ||
|
|
||
| function sendNotificationIfAllowed(title, options) { |
There was a problem hiding this comment.
Maybe sendNotificaitonIfEnabled?
…question is indeed marked being answered
| <ModalBody> | ||
| <h5>For students</h5> | ||
| <p> | ||
| If you enable notificaitons, you'll get notifications when your |
There was a problem hiding this comment.
"If you enable notificaitons" => notifications
There was a problem hiding this comment.
Sorry, what do you mean exactly? Do you mean "Notificaitons: you'll get notifications when your..." like this?
There was a problem hiding this comment.
oh sorry, i meant "If you enable notificaitons" should be changed to "If you enable notifications", the current notifications is a typo
There was a problem hiding this comment.
Oh, right! I can fix that later along with the wording stuff I mentioned below.
There was a problem hiding this comment.
Please double check!
| If you enable notificaitons, you'll get notifications when your | ||
| question is being answered. | ||
| </p> | ||
| <h5>For course staff/admins</h5> |
There was a problem hiding this comment.
Could these be wrapped and be shown based on user role (course staff vs not course staff)?
There was a problem hiding this comment.
I had a discussion with Nathan, and we agreed that probably it is a better choice to just show both messages because it's hard to handle edge cases properly when distinguishing roles. For example, Nathan is an admin, but also a student in CS 461. But it's definitely better if we can handle the role more properly.
There was a problem hiding this comment.
ahhh, makes sense
| If you enable notificaitons, you'll get notifications when your | ||
| question is being answered. | ||
| </p> | ||
| <h5>For course staff/admins</h5> |
There was a problem hiding this comment.
@nwalters512 Do you mind if I change it to For on-duty couse staff/admins, since that is more aligned with our actual logic. You don't get notifications if you don't put yourself on the on-duty staff list.
Fix a typo, improve modal text
Fixes #80, let me know if I'm doing anything stupid :(