Add watchers #20

milki · 2013-06-24T22:36:42Z

A watcher or list of watchers can be assigned to pull requests. Watchers are listed as alternate owners of the request and are pinged wherever the original requester would be pinged.

This PR includes a new test framework for templates. In doing so, some templates were refactored for easier template testing.

Unfortunately, the javascript changes do not have automatic tests. They have been verified through smoke testing with a local test instance and IRC.

Note: This also encompasses the feature request in PR #16

TemplateTestCase provides the bare minimum needed to test templates through rendering.

A watcher or list of watchers can be assigned to pull requests. Watchers are listed as alternate owners of the request and are pinged wherever the original requester would be pinged.

Additionally protect push buttons from random users

authorized_to_manage_requests returns True for: request owner request watchers pushmaster

class="mine" is a visual indicator for request ownership in the request lists. Include watchers as well.

milki · 2013-06-27T17:47:10Z

tests/test_template_request.py

+    requester_edit_button_text = ['Edit', 'Delay', 'Discard']
+    edit_button_classes = ['edit-request']
+    edit_button_text = ['Edit']
+


All these buttons need to be verified to be displayed on the website as well. Some of these don't show up because of CSS. For example, the push_button_classes aren't showing up for pushmaster.

Only made un-pickme visible in the pickme state with 8a0e3a3. The other one's are redundant with the pushmaster-only buttons.

This might be testable with something like webdriver + phantomjs, which would be super cool but is probably out of scope for now.

mrtyler · 2013-07-09T23:16:22Z

static/js/push.js

@@ -87,7 +87,7 @@ $(function() {
    });
    $('.message-people').live('click', function() {
        var contents = $(this).siblings('.item-count').text();
-        var people = (/(?:[a-z]+,?\s?)+/.exec(contents) || [""])[0];
+        var people = (/(?:[a-z]+(?:\s\((?:[a-z]+,?\s?)+\))?,?\s?)+/.exec(contents) || [""])[0];


Uh... can we unpack this? This is crazypants.

Does js allow multi-line regex or anything else to make this easier to digest? At least can we separate the regex insanity from the other stuff that's going on (calling exec, assigning to people)?

I've found that vanilla javascript doesnt support the /x verbose mode of regexes though there is an alternative XRegexp.

That's fine about /x. lol javascript.

Please still unpack the other complexity from the line.

mrtyler · 2013-07-10T00:02:11Z

This looks good, modulo my complaints above. I really like the template refactoring and template unit tests!

baris · 2013-07-28T06:18:19Z

These changes look good to me. @milki++ for template tests! What's the plan to resolve the conflict & merge this in?

milki · 2013-07-29T04:20:59Z

I'll merge master into watchers and fix conflicts.

milki · 2013-07-29T21:33:28Z

Also, I need to provide a sqlscript for both sqlite3 and mysql and test an actual migration.

Conflicts: testing/testservlet.py tests/test_template_newrequest.py

milki · 2013-07-29T23:46:20Z

static/js/push.js

+        var people_pat = new RegExp(    // person, person (person, person), person (person), person
+            "(?:[a-z]+" +                   // A username, possibly followed by:
+                "(?:\\s\\(" +               //   a space and (
+                    "(?:[a-z]+,?\\s?)+" +   //   and or more comma seperated usernames


Probably meant to say one or more here. Also probably good to mention this in the form.

milki · 2013-07-31T20:50:33Z

I've realized I've also forgotten to email watchers on request status changes.

milki · 2013-08-07T05:48:13Z

There are two outstanding issues beyond are beyond the scope of this PR:

#26 Duplicate emails which needs to be investigated.

#25 Prevents testing for the CommentRequest servlet. Thus, it has not been included in this PR.

As mentioned above, javascript testing and template styling issues is beyond the scope of this PR as well.

Other than the above issues which can be addressed later, this PR is ready for review.

milki · 2013-08-07T05:48:53Z

UPDATING

@@ -1,3 +1,14 @@
+2013.07.29
+  AFFECTS: Users with existing installs before commit $add_watchers-merge$


TODO: Update this to the merge commit sha after the merge.

baris · 2013-08-12T23:06:58Z

Ship it!

A watcher or list of watchers can be assigned to pull requests. Watchers are listed as alternate owners of the request and are pinged wherever the original requester would be pinged.

ymilki added 11 commits June 27, 2013 09:39

Add TemplateTestCase

b3cea38

TemplateTestCase provides the bare minimum needed to test templates through rendering.

Refactor request-info out of request template

9cd3a41

Add test for labels/inputs in newrequest form

3517787

Test user title in request-info template

c2f2a0d

Add watchers

437ec48

A watcher or list of watchers can be assigned to pull requests. Watchers are listed as alternate owners of the request and are pinged wherever the original requester would be pinged.

refactor test_template_request

dd5b97f

Breakout request buttons for testing

d64be03

Additionally protect push buttons from random users

Add ui_methods and authorized_to_manage_request

c569805

authorized_to_manage_requests returns True for: request owner request watchers pushmaster

Allow watchers to manage requests in pull

37e17f5

Refactor push-status from push templates

2b869af

Label requests as "mine" for watchers

5880cac

class="mine" is a visual indicator for request ownership in the request lists. Include watchers as well.

milki reviewed Jun 27, 2013
View reviewed changes

Show unpickme to pushmaster

8a0e3a3

milki mentioned this pull request Jul 2, 2013

Restrict verify to pushmaster and user #16

Closed

mrtyler reviewed Jul 9, 2013
View reviewed changes

ymilki added 3 commits July 19, 2013 14:57

Load ui_modules/ui_methods the proper way

b8f1bb5

varname typo

1e19932

verbose regex

90d10b0

ymilki added 2 commits July 29, 2013 16:33

Merge branch 'master' into add_watchers

7931803

Conflicts: testing/testservlet.py tests/test_template_newrequest.py

Add sql migration and UPDATING

11a5b00

milki reviewed Jul 29, 2013
View reviewed changes

Clarify watchers list format

3b1e6ed

ymilki added 3 commits August 5, 2013 18:47

Sends emails/xmpp to watchers too

d58d28f

fix whitespace

9b2e0bb

UPDATING can't mention commit sha yet

1f473c8

milki mentioned this pull request Aug 7, 2013

addrequest/discardrequest send duplicate emails #26

Open

milki reviewed Aug 7, 2013
View reviewed changes

milki merged commit 1f473c8 into Yelp:master Aug 14, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add watchers #20

Add watchers #20

milki commented Jun 24, 2013

milki Jun 27, 2013

milki Jun 27, 2013

mrtyler Jul 9, 2013

mrtyler Jul 9, 2013

milki Jul 16, 2013

mrtyler Jul 17, 2013

mrtyler commented Jul 10, 2013

baris commented Jul 28, 2013

milki commented Jul 29, 2013

milki commented Jul 29, 2013

milki Jul 29, 2013

milki commented Jul 31, 2013

milki commented Aug 7, 2013

milki Aug 7, 2013

baris commented Aug 12, 2013

		@@ -1,3 +1,14 @@
		2013.07.29
		AFFECTS: Users with existing installs before commit $add_watchers-merge$

Add watchers #20

Add watchers #20

Conversation

milki commented Jun 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrtyler commented Jul 10, 2013

baris commented Jul 28, 2013

milki commented Jul 29, 2013

milki commented Jul 29, 2013

Choose a reason for hiding this comment

milki commented Jul 31, 2013

milki commented Aug 7, 2013

Choose a reason for hiding this comment

baris commented Aug 12, 2013