Merge lp://qastaging/~qzhang/lava-dashboard/user-notification into lp://qastaging/lava-dashboard

Proposed by Spring Zhang
Status: Merged
Merged at revision: 337
Proposed branch: lp://qastaging/~qzhang/lava-dashboard/user-notification
Merge into: lp://qastaging/lava-dashboard
Diff against target: 510 lines (+402/-2)
8 files modified
dashboard_app/forms.py (+11/-1)
dashboard_app/migrations/0014_auto__add_notification.py (+226/-0)
dashboard_app/models.py (+73/-1)
dashboard_app/templates/dashboard_app/failure_summary_mail.txt (+13/-0)
dashboard_app/templates/dashboard_app/notification_pref.html (+31/-0)
dashboard_app/urls.py (+2/-0)
dashboard_app/views.py (+45/-0)
doc/changes.rst (+1/-0)
To merge this branch: bzr merge lp://qastaging/~qzhang/lava-dashboard/user-notification
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle (community) Needs Fixing
Spring Zhang (community) Needs Resubmitting
Review via email: mp+113167@code.qastaging.launchpad.net

Description of the change

Add user notification preference page on bundle stream selection

1. Ctrl+left mouse button click to get multiple selections on bundle streams.
2. One bug is when user is Guest, it will issue an error.

To post a comment you must log in.
339. By Spring Zhang

correct some errors

340. By Spring Zhang

Adjust signal bundle_was_deserialized sending time to avoid following error:

1. get_summary_result() return:
'NoneType' object is not subscriptable
because bundle is not saved yet.

Revision history for this message
Spring Zhang (qzhang) wrote :

Fix some errors

Revision history for this message
Spring Zhang (qzhang) wrote :

Fix some errors and tried on local deployment

Revision history for this message
Spring Zhang (qzhang) wrote :

tested

review: Needs Resubmitting
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Hi, this is a good start. There are some things that should be improved before landing though I think:

1) I think the notification_list view should be marked login_required.
2) I don't see the notification page being added to ME_PAGE_ACTIONS -- did you mean to merge my branch or reimplement this yourself?
3) Can you make the box from which you select the bundle streams to subscribe to much bigger? I guess you can do this just using CSS if there's no neat way to do this in Django.
4) Can you add Notification to the admin interface? (See admin.py) I always forget to do this!
5) You might want to add a unique_together constraint to Notification (https://docs.djangoproject.com/en/dev/ref/models/options/#unique-together) -- this will eliminate the need for the fixme you have about MultipleObjectsReturned
6) The calculation of 'init' in the view function is querying the wrong table -- you want to query the bundlestreams that 'request.user' is already subscribed to, which you can do like this:

    init = BundleStream.objects.filter(
        notification__user=request.user, notification__if_notify=True)

7) You need to set the existing notifications to if_notify=False _before_ you set the ones in the form to true -- otherwise if_notify is always false!

So that's quite a lot of comments sorry, but I don't think any of them should be too hard to address. I haven't tested the emailing code yet, but that _looks_ OK...

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches