Merge lp:~danilo/launchpad/subscribe-unsubscribe into lp:launchpad
Status: | Merged |
---|---|
Approved by: | Данило Шеган |
Approved revision: | no longer in the source branch. |
Merged at revision: | 12953 |
Proposed branch: | lp:~danilo/launchpad/subscribe-unsubscribe |
Merge into: | lp:launchpad |
Diff against target: |
65 lines (+13/-5) 1 file modified
lib/lp/bugs/javascript/bugtask_index_portlets.js (+13/-5) |
To merge this branch: | bzr merge lp:~danilo/launchpad/subscribe-unsubscribe |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Abel Deuring (community) | code | Approve | |
Review via email: mp+59511@code.launchpad.net |
Commit message
[r=adeuring][bug=770293] Allow unsubscribing from the bug page even without page reload when you subscribed inline.
Description of the change
= Bug 770293 =
After one subscribes to a bug, his user link and unsubscription icon is added to the subscribers list. However, unsubscribing action doesn't work because user is tagged as the "dupe subscriber" on subscribing, which causes the unsubscribe action to call unsubscribeFrom
= Implementation details =
The actual fix for this is addition of "subscription.
Everything else is drive-by fixes:
- lines ~486: hide_spinner emitted a "failed" event, and was also called from on_success handler which emitted a "success" event; this caused unsubscribe action to be called twice because two "click" handlers were set-up, which caused a lot of confusion during debugging - the fix is to split out hide_spinner from the failure handler
- lines ~610: the check if there are no other entries works better if we do it before we remove one of the entries (there can be at most two person links: one for direct, another for duplicate subscription), especially in a debugger (which can block things like animations); so, instead of checking if there are no links anymore after removal, we check if there was exactly one link before removal
- lines ~1054: typo fix
- lines ~1357: when someone "subscribes someone else" to a bug, an entry for them is added to the subscribers list; if they are a team, and current user is a member of that team, they should be able to unsubscribe them. However, this code was using the attribute directly which doesn't exist, so when fixed to use get(), it started working.
= Tests =
There is no easy way to test this, and since I am still doing some refactoring of the code in bugtask_
= Demo & QA =
With the lack of tests, QA is very important for this. I seriously hope that we'll have enough time to introduce proper tests for this or it's inevitably going to break again in similar ways.
== For the actual bug ==
Go to https:/
"Subscribe" yourself, "Unsubscribe" yourself (eg. using the "(-)" icon next to your name in the subscribers list), and reload to make sure that you have been unsubscribed.
== For drive-by fixes ==
1. Try subscribing the same team to a bug, and to a duplicate of the bug. From the 'primary' bug, unsubscribe a team from the duplicate, and from the bug using "(-)" icons
2. Observe that the unsubscribe (-) icon is added for these teams even without page reload
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
(15:45:52) adeuring: danilos: nice work. Just one minor remark: for me, the variable name "person_links" sounds more like a sequence, but it contains the number of links. What about renaming it to num_person_links?
(15:45:56) adeuring: but anyway, r0me
(15:46:00) adeuring: r=me
(15:46:19) danilos: adeuring, heh, I know, but I'll have to wrap the line then :)
(15:46:24) danilos: adeuring, thanks, I'll fix it
(15:46:49) adeuring: danilos: I see your point ;) leave it if you think it looks nicer ;)