Watch tree 842340 #3602

groovecoder · 2015-10-26T21:36:14Z

Spot-check instructions:

Sign in
Go to a doc with a sub-article
Under the advanced menu, click the "Subscribe to this article and all its sub-articles"
- Verify the subscription notification UI is correct
Sign in as a different user
Go to the sub-article
Edit the sub-article
Check the ./manage.py worker console output.
- Verify our notification email is sent to the user who subscribed to the article + sub-articles. E.g.:

...
Subject: [MDN] Page "groovechild" changed by DavidWalshOS
From: notifications@developer.mozilla.org
...
--
You are subscribed to edits on: User:groovecoder and all its sub-articles.
Unsubscribe from these emails:
https://example.com/unsubscribe/14?s=dCvbUfbpab

BONUS spot-check waffle flag:
8. Add a waffle flag called watch_menu and activate it for a user
9. Visit a doc with a sub-article as the user

Verify there's a new UI "eye" element with the Subscribe links inside
Tests
Analytics tracking for split-testing
Bug 1218448 - Subscribe to article: notification never dismisses
Feedback from @stephaniehobson on this UI approach: moving all of the "subscribe" page items into their own page nav menu:

On non-English pages, there are 3 items in the menu:

stephaniehobson · 2015-10-27T04:18:21Z

TL;DR I like this direction I want to A/B test to make sure we don't lose edits to clutter.

I don't know if it's a good idea to be adding buttons to the interface. Another display option here would be to separate them within the menu using a dotted line.

But I know that you know that subscriptions increase return visits so I'm not sure how to weigh more subscriptions vs clutter. Maybe we could get @hoosteeno to weigh in?

If we're going to increase the prominence of these options now's a good time to play around with them. There are so many good opportunities for A/B testing here:

"subscribe to this article" vs "email me when this article updates"
eye icon vs feed icon vs email icon (the eye icon is occasionally used for accessibility options probably not often enough that it's a problem but worth testing)

jezdez · 2015-10-27T09:12:13Z

Minor nit, I think "children" is a bit of a misleading term for describing subpages as it implies that users know our data tree model. Could we call it "section"? Or maybe "subpages"?

groovecoder · 2015-10-27T12:13:08Z

Great comments.

I'll re-use the term "sub-articles" that we use in the advanced menu. (i.e., "New sub-article")

I'll start by split-testing between:

Separate "Subscribe" menu with eye icon
"Subscribe" menu options in the Advanced menu, separated by a dotted line

Let's test that for a week or more, then look at testing verbage and iconography.

@stephaniehobson - do we already have an element (<hr/>?) or class for an <li> separator?

groovecoder · 2015-10-27T12:55:39Z

Updated with a watch_menu flag to allow us to split-test ...

Need to add tests now.

groovecoder · 2015-10-28T20:46:39Z

Okay, this is ready for final review & spot-checking. Who wants it? @stephaniehobson for front-end & spot-check? @robhudson, @jwhitlock, or @jezdez for back-end code review?

groovecoder · 2015-10-28T21:00:49Z

Also rebased this on latest master.

stephaniehobson · 2015-10-28T21:08:17Z

If you put two lists next to each other in a navigation drop down the border should appear automatically, sorry, should have said.

stephaniehobson · 2015-10-28T21:54:11Z

includes the link to subscribe to sub-articles but it has no sub-articles

Some tags around the language will save us some RTL headaches.

Subscribe to <bdi>English (US)</bdi> version

After subscribing to a tree, when visiting a child article there is no indication for the user that they are already subscribed to that article. Is it even possible to subscribe to a tree and than unsubscribe from a child? If the user is subscribed to both the tree and the child will they get duplicate email notifications? Is this a toggle - if I subscribe to the tree and then subscribe to the article have I actually unsubscribed?

I think we can answer some of these questions by including some text in the menu if they are subscribed to the tree already. Something like "You are subscribed to this article's parent and all of its children", only more clear and friendly than what I scribble in a github comment box 8 minutes before I have to leave to catch a flight.

Wasn't able to test the emails actually sending, happy to do it on Friday if it still needs doing but I'll need more detailed steps. Anyway, going to go get on a plane now.

verify expected emails in tests

robhudson · 2015-10-30T15:32:43Z

kuma/wiki/templates/wiki/email/edited.ltxt

+      {{ _('You are subscribed to edits on: {title} and all its sub-articles.')|f(title=title) }}
+    {% else %}
+      {{ _('You are subscribed to edits on: {title}.')|f(title=title) }}
+    {% endif %}


Since this is an email, do you want to use jinja2 whitespace eating tags? Or do you want those strings indented?

groovecoder · 2015-10-30T17:19:29Z

So, the first implementation I have for showing the parent tree subscription(s) to the user is simply putting an "Unsubscribe" link for each parent tree the user is watching in the menu. It's not pretty, and it could get fugly if users watch multiple trees above a doc. (Though I'm not sure why they would?)

Would probably like some better UX/UI direction before merging this. 😐 @stephaniehobson - more suggestions?

jezdez · 2015-11-02T14:30:26Z

kuma/wiki/tests/test_views.py

+                         data)
+        self.assertEquals(2, len(mail.outbox))
+        message = mail.outbox[0]
+        assert testuser2.email in message.to


any reason to not use self.assertIn here? I'm all for using native assert statements in tests, but that'd be a third option of conventions in our tests. I'd rather move us to pure Django tests that offer stuff like self.assertIn :-/

In today's backend meeting we agreed to start removing nose assertion helpers to cut down on the different number of conventions in our tests. Going to leave this as-is unless/until we start removing native asserts.

groovecoder · 2015-11-02T18:01:08Z

Assigning to @stephaniehobson to review (and hopefully not puke from) my proposed just-stick-it-in-the-menu UX approach.

stephaniehobson · 2015-11-09T23:18:07Z

Needs a rebase :/

stephaniehobson · 2015-11-09T23:56:22Z

Small problem with the HTML escaping:

Suggestions (not blocking):

put the article title in italics in the menu
re-order the menus so that events relating to the page the user is on are grouped together (ie, Add detail to setup description #1 is always (un)subscribe to this article and Bug 674755 #2 is always (un)subscribe to this article and sub-articles)

R+ after rebase, changes are optional.

stephaniehobson · 2015-11-09T23:56:40Z

Oh, except fixing the problem with the HTML escaping. That's not optional.

Conflicts: kuma/wiki/events.py kuma/wiki/views/document.py

groovecoder · 2015-11-10T17:14:46Z

Merge was easier than rebase. Looking into the rest ...

move parent tree watch links below page watch links

groovecoder · 2015-11-10T18:27:11Z

Escaped the doc title in tree menu items.
Moved tree items below page items.
Italicizing the page title and escaping it at the same time will be tricky, so I'm leaving that for a follow-up enhancement.

groovecoder · 2015-11-10T22:26:54Z

Assigning to @stephaniehobson to double-check my updates. Then this is good to merge.

stephaniehobson · 2015-11-12T22:20:12Z

Quotes around the article name instead of italics would also be an improvement, but not one to block on ;)

R+

Watch tree 842340

groovecoder · 2015-11-23T14:39:32Z

After the first week of ~50/50 split testing ...

The separate watch menu doesn't seem to have much effect on users clicking the "Edit" button. Conversion rate with the separate watch menu is 0.04% vs. 0.03% with the watch items included in the "Advanced" menu. (Note: I haven't done a full statistical analysis on the level of Optimizely testing.)

In addition, the extra menu doesn't seem to have much effect on users going to the "Translate" screen either. Conversion rate with the separate watch menu is 0.03% vs. 0.02% with the watch items included in the "Advanced" menu. (Again, I haven't done a full statistical analysis on the level of Optimizely testing.)

There even less data for the new "Watch" goal, but there are 2x as many conversions with the separate watch menu so far.

With all that said, I'm leaning towards keeping the separate watch menu and removing the items from the advanced menu. But, let's give it another week to collect some more data.

groovecoder added not ready labels Oct 26, 2015

groovecoder force-pushed the watch-tree-842340 branch 2 times, most recently from 261bbfc to 6792b70 Compare October 27, 2015 12:49

groovecoder force-pushed the watch-tree-842340 branch from 4d0102c to 0f46222 Compare October 28, 2015 01:50

groovecoder added analytics and removed needs tests labels Oct 28, 2015

groovecoder added 10 commits October 28, 2015 15:48

bug 842340 - new tidings event for trees

cca5fde

bug 842340 - fire a single EventUnion on edits

3fb6ee0

bug 842340 - review cleanup

70eac0d

bug 842340 - watch_menu_items macro for buttons.html

c8f90d1

bug 842340: tree subscription copy in edited.ltxt

052042e

bug 842340: watch tree tests: events, models, views

0280387

bug 842340 - view tests for document watches

9d2d89b

bug 842340 - GA tracking for page watch (menu)

c58e966

bug 842340 - flake8 test code; context for macro

f3cee25

fix bug 1218448 - no need to JSON.parse data

3e4ac84

groovecoder force-pushed the watch-tree-842340 branch from 51adf27 to 3e4ac84 Compare October 28, 2015 21:00

jwhitlock self-assigned this Oct 28, 2015

bug 842340 - proper <ul> nesting of watch_menu

176be50

bug 842340 - clean edited.ltxt footer

cfa69a3

verify expected emails in tests

robhudson reviewed Oct 30, 2015
View reviewed changes

bug 842340 - clean whitespace in emails

fabc188

groovecoder force-pushed the watch-tree-842340 branch from 1d7c154 to fabc188 Compare October 30, 2015 17:15

groovecoder added 2 commits October 30, 2015 12:54

bug 842340 - include parent tree unsubscribe links

a418f65

bug 842340 - rename document tree test helper

c6f7345

jezdez reviewed Nov 2, 2015
View reviewed changes

groovecoder assigned stephaniehobson and unassigned robhudson Nov 2, 2015

stephaniehobson assigned groovecoder and unassigned stephaniehobson Nov 9, 2015

Merge branch 'master' into watch-tree-842340

01a06c1

Conflicts: kuma/wiki/events.py kuma/wiki/views/document.py

bug 842340 - escape doc title in tree item

50ad728

move parent tree watch links below page watch links

groovecoder force-pushed the watch-tree-842340 branch from c835976 to 50ad728 Compare November 10, 2015 18:23

groovecoder assigned stephaniehobson and unassigned groovecoder Nov 10, 2015

stephaniehobson added a commit that referenced this pull request Nov 12, 2015

Merge pull request #3602 from mozilla/watch-tree-842340

e833a8b

Watch tree 842340

stephaniehobson merged commit e833a8b into master Nov 12, 2015

groovecoder deleted the watch-tree-842340 branch November 23, 2015 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Watch tree 842340 #3602

Watch tree 842340 #3602

groovecoder commented Oct 26, 2015

stephaniehobson commented Oct 27, 2015

jezdez commented Oct 27, 2015

groovecoder commented Oct 27, 2015

groovecoder commented Oct 27, 2015

groovecoder commented Oct 28, 2015

groovecoder commented Oct 28, 2015

stephaniehobson commented Oct 28, 2015

stephaniehobson commented Oct 28, 2015

robhudson Oct 30, 2015

groovecoder commented Oct 30, 2015

jezdez Nov 2, 2015

groovecoder Nov 2, 2015

groovecoder commented Nov 2, 2015

stephaniehobson commented Nov 9, 2015

stephaniehobson commented Nov 9, 2015

stephaniehobson commented Nov 9, 2015

groovecoder commented Nov 10, 2015

groovecoder commented Nov 10, 2015

groovecoder commented Nov 10, 2015

stephaniehobson commented Nov 12, 2015

groovecoder commented Nov 23, 2015

Watch tree 842340 #3602

Watch tree 842340 #3602

Conversation

groovecoder commented Oct 26, 2015

stephaniehobson commented Oct 27, 2015

jezdez commented Oct 27, 2015

groovecoder commented Oct 27, 2015

groovecoder commented Oct 27, 2015

groovecoder commented Oct 28, 2015

groovecoder commented Oct 28, 2015

stephaniehobson commented Oct 28, 2015

stephaniehobson commented Oct 28, 2015

robhudson Oct 30, 2015

Choose a reason for hiding this comment

groovecoder commented Oct 30, 2015

jezdez Nov 2, 2015

Choose a reason for hiding this comment

groovecoder Nov 2, 2015

Choose a reason for hiding this comment

groovecoder commented Nov 2, 2015

stephaniehobson commented Nov 9, 2015

stephaniehobson commented Nov 9, 2015

stephaniehobson commented Nov 9, 2015

groovecoder commented Nov 10, 2015

groovecoder commented Nov 10, 2015

groovecoder commented Nov 10, 2015

stephaniehobson commented Nov 12, 2015

groovecoder commented Nov 23, 2015