1061592 - [User Story] As a helper I can select a question so that I can add a comment and get visual confirmation of submission.

Status: VERIFIED FIXED

(support.mozilla.org :: BuddyUp, defect)

Comment 1

[Michael Cooper [:mythmon]]

Adding implementation bugs as blockers.

Comment 3

[Schalk Neethling [:espressive]]

* The “Receive new comment notifications” checkbox gets automatically checked so that the helper starts receiving notifications on all comments related to this discussion thread.

NOTE: From looking at the UX doc, it looks like the checkbox “Receive new...." will be checked be default.

This then means that after a helpee asked their first question, or a helper comments we need to set new_comment_notification to true for the relevant user.

Comment 4

[Schalk Neethling [:espressive]]

The branch that contains the relevant work to add the, receive new comment notification field, and handle the update of the preference on the server and locally, lives here for now:

https://github.com/ossreleasefeed/buddyup/tree/bug1061592-add-new-comments-notification-checkbox

Comment 5

[Schalk Neethling [:espressive]]

A note about the above. This sends a PATCH request to the server to update the preference but, at the moment the user object is missing the properties/fields for:

new_comment_notify
new_questions_notify

so, on the server side, the value is not being stored yet.

Comment 6

[Michael Cooper [:mythmon]]

Those will be "user settings" which work similarly to the question metadata. The API for them isn't very good, but I couldn't convince DRF to do better. This will be landing soon, and I hope to be able to write some basic docs for these soon.

Comment 7

[Schalk Neethling [:espressive]]

Attachment: https://github.com/mozilla/buddyup/pull/41

Comment 8

[Schalk Neethling [:espressive]]

Just FYI, the PR is against the correct bug number. It relates to:
"* The “Receive new comment notifications” checkbox gets automatically checked so that the helper starts receiving notifications on all comments related to this discussion thread."

I know that this is not related to a sprint specifically but, it is work that has been done, would be a shame to loose it ;)

Comment 9

[Anthony Ricaud (:rik)]

Comment on attachment 8542127 [details] [review]
https://github.com/mozilla/buddyup/pull/41

Although the code looks ok, it's not implementing something we want. The checkbox on a question is to disable notifications for this particular question. Here, you're disabling notifications for every question by changing the user global setting. This is bug 1061595.

Comment 10

[Anthony Ricaud (:rik)]

To disable notifications for a question, it's bug 1105767 and bug 1066386.

So I think we can close this again and work in one of those three bugs.

Comment 11

[Schalk Neethling [:espressive]]

I agree with all of that, but I believe the question here is, is the code good to merge or not. It lays the groundwork, no need to throw it away?

Comment 12

[Anthony Ricaud (:rik)]

Well it's not landable since it's not implementing the right feature. But it's not thrown away either, we can reuse parts of it for the correct features.

Comment 13

[Schalk Neethling [:espressive]]

Ah yes, I see those individual bugs now but, a lot of the work I have been doing depends on work I did in https://github.com/mozilla/buddyup/pull/41 so, it would be great if we could get that merged in.

I know the checkbox does not function as it should, but changing that will be a lot easier than changing everything else I have done that followed on from that. You will for example see the update_preference function come to use again in https://github.com/mozilla/buddyup/pull/46

I really hope we can work this out, get this in and move forward.