From 62d9eba5f7eb364d2284cc5132ac5e31756f43e6 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Dec 02 2016 09:45:58 +0000 Subject: [PATCH 1/6] Issue 1620 - list subscribers on the Issue/PR pages --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 5c72474..c20ccba 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3344,6 +3344,38 @@ def is_watching_obj(session, user, obj): return False +def get_watch_list(session, obj): + """ Return a list of all the users that are watching the "object" + """ + if obj.isa == "issue": + query = session.query( + model.IssueWatcher + ).filter( + model.IssueWatcher.issue_uid == obj.uid + ) + elif obj.isa == "pull-request": + query = session.query( + model.PullRequestWatcher + ).filter( + model.PullRequestWatcher.pull_request_uid == obj.uid + ) + else: + raise pagure.exceptions.InvalidObjectException( + 'Unsupported object found: "%s"' % obj + ) + + user_objs = query.all() + users = [] + if user_objs: + # We found watchers + for user in user_objs: + users.append(str(user.user.username)) + if str(obj.user.user) not in users: + # Add Issue/PR creator if not already in the list + users.append(str(obj.user.user)) + return users + + def save_report(session, repo, name, url, username): """ Save the report of issues based on the given URL of the project. """ diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html index 4b25403..56efa0c 100644 --- a/pagure/templates/issue.html +++ b/pagure/templates/issue.html @@ -312,6 +312,19 @@ {% if authenticated %} + {% if subscribers %} +
+
+
+ +
+ {{ subscribers | join(', ') }} +
+
+
+
+ {% endif %} +
+ + {% if subscribers and authenticated %} +
+
+
+ +
+ {{ subscribers | join(', ') }} +
+
+
+
+ {% endif %} + {% endif %} {% if diff and pull_request%} diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 99b4054..c1837b8 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -317,6 +317,7 @@ def request_pull(repo, requestid, username=None, namespace=None): diff_commits=diff_commits, diff=diff, mergeform=form, + subscribers=pagure.lib.get_watch_list(SESSION, request), ) diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 8447598..416d04e 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -894,6 +894,7 @@ def view_issue(repo, issueid, username=None, namespace=None): form=form, knowns_keys=knowns_keys, subscribed=subscribed, + subscribers=pagure.lib.get_watch_list(SESSION, issue), ) From 71aaa20f5ca04ef21c6414eccf4d40c8326afa32 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 02 2016 09:45:58 +0000 Subject: [PATCH 2/6] Rework pagure.lib.get_watch_list() to cover all the cases --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index c20ccba..f783e96 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3348,13 +3348,13 @@ def get_watch_list(session, obj): """ Return a list of all the users that are watching the "object" """ if obj.isa == "issue": - query = session.query( + obj_watchers_query = session.query( model.IssueWatcher ).filter( model.IssueWatcher.issue_uid == obj.uid ) elif obj.isa == "pull-request": - query = session.query( + obj_watchers_query = session.query( model.PullRequestWatcher ).filter( model.PullRequestWatcher.pull_request_uid == obj.uid @@ -3364,16 +3364,50 @@ def get_watch_list(session, obj): 'Unsupported object found: "%s"' % obj ) - user_objs = query.all() - users = [] - if user_objs: - # We found watchers - for user in user_objs: - users.append(str(user.user.username)) - if str(obj.user.user) not in users: - # Add Issue/PR creator if not already in the list - users.append(str(obj.user.user)) - return users + project_watchers_query = session.query( + model.Watcher + ).filter( + model.Watcher.project_id == obj.project.id + ) + + users = set() + + # Add the person who opened the object + users.add(obj.user.username) + + # Add all the people who commented on that object + for comment in obj.comments: + users.add(comment.user.username) + + # Add the user of the project + users.add(obj.project.user.username) + + # Add the regular contributors + for contributor in obj.project.users: + users.add(contributor.username) + + # Add people in groups with commit access + for group in obj.project.groups: + for member in group.users: + users.add(member.username) + + # Add all the people watching the repo, remove those who opted-out + for watcher in project_watchers_query.all(): + if watcher.watch: + users.add(watcher.user.username) + else: + if watcher.user.username in users: + users.remove(watcher.user.username) + + # Add all the people watching this object, remove those who opted-out + for watcher in obj_watchers_query.all(): + if watcher.watch: + users.add(watcher.user.username) + else: + if watcher.user.username in users: + users.remove(watcher.user.username) + + return users def save_report(session, repo, name, url, username): From bed95662829c96b40778dea4d115c5bc9f15e2f3 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 02 2016 09:45:58 +0000 Subject: [PATCH 3/6] Drop the call to pagure.lib.is_watching_obj, rely on the watch_list instead --- diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html index 56efa0c..cfea33a 100644 --- a/pagure/templates/issue.html +++ b/pagure/templates/issue.html @@ -325,9 +325,9 @@ {% endif %} -
+
From 7e0d4414964fb6a8968be1dcc37745b32f769c3c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 02 2016 09:45:58 +0000 Subject: [PATCH 5/6] Drop pagure.lib.is_watching_obj and adjust the unit-tests accordingly --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index f783e96..38886c1 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3291,59 +3291,6 @@ def set_watch_obj(session, user, obj, watch_status): return output -def is_watching_obj(session, user, obj): - ''' Check if the user is watching the specified object. - - Objects can be either an issue or a pull-request - ''' - - if not isinstance(user, model.User): - try: - user = get_user(session, user) - except pagure.exceptions.PagureException: - return False - - if not user: - return False - - # First check if the user explicitely turned on/off notifications - if obj.isa == "issue": - query = session.query( - model.IssueWatcher - ).filter( - model.IssueWatcher.user_id == user.id - ).filter( - model.IssueWatcher.issue_uid == obj.uid - ) - elif obj.isa == "pull-request": - query = session.query( - model.PullRequestWatcher - ).filter( - model.PullRequestWatcher.user_id == user.id - ).filter( - model.PullRequestWatcher.pull_request_uid == obj.uid - ) - else: - raise pagure.exceptions.InvalidObjectException( - 'Unsupported object found: "%s"' % obj - ) - - watcher = query.first() - - if watcher: - return watcher.watch - - # Otherwise, just check if they are in the default group - if obj.user.user == user.user: - return True - - for comment in obj.comments: - if comment.user.user == user.user: - return True - - return False - - def get_watch_list(session, obj): """ Return a list of all the users that are watching the "object" """ diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index e044330..dec2bb1 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -1587,8 +1587,22 @@ class PagureFlaskApiIssuetests(tests.Modeltests): p_send_email.return_value = True p_ugt.return_value = True + item = pagure.lib.model.User( + user='bar', + fullname='bar foo', + password='foo', + default_email='bar@bar.com', + ) + self.session.add(item) + item = pagure.lib.model.UserEmail( + user_id=3, + email='bar@bar.com') + self.session.add(item) + + self.session.commit() + tests.create_projects(self.session) - tests.create_tokens(self.session) + tests.create_tokens(self.session, user_id=3) tests.create_tokens_acl(self.session) headers = {'Authorization': 'token aaabbbcccddd'} @@ -1646,8 +1660,9 @@ class PagureFlaskApiIssuetests(tests.Modeltests): # Check subscribtion before repo = pagure.lib.get_project(self.session, 'test') issue = pagure.lib.search_issues(self.session, repo, issueid=1) - self.assertFalse( - pagure.lib.is_watching_obj(self.session, 'pingou', issue)) + self.assertEqual( + pagure.lib.get_watch_list(self.session, issue), + set(['pingou', 'foo'])) # Unsubscribe - no changes @@ -1674,8 +1689,9 @@ class PagureFlaskApiIssuetests(tests.Modeltests): # No change repo = pagure.lib.get_project(self.session, 'test') issue = pagure.lib.search_issues(self.session, repo, issueid=1) - self.assertFalse( - pagure.lib.is_watching_obj(self.session, 'pingou', issue)) + self.assertEqual( + pagure.lib.get_watch_list(self.session, issue), + set(['pingou', 'foo'])) # Subscribe data = {'status': True} @@ -1701,8 +1717,9 @@ class PagureFlaskApiIssuetests(tests.Modeltests): repo = pagure.lib.get_project(self.session, 'test') issue = pagure.lib.search_issues(self.session, repo, issueid=1) - self.assertTrue( - pagure.lib.is_watching_obj(self.session, 'pingou', issue)) + self.assertEqual( + pagure.lib.get_watch_list(self.session, issue), + set(['pingou', 'foo', 'bar'])) # Unsubscribe data = {} @@ -1717,8 +1734,9 @@ class PagureFlaskApiIssuetests(tests.Modeltests): repo = pagure.lib.get_project(self.session, 'test') issue = pagure.lib.search_issues(self.session, repo, issueid=1) - self.assertFalse( - pagure.lib.is_watching_obj(self.session, 'pingou', issue)) + self.assertEqual( + pagure.lib.get_watch_list(self.session, issue), + set(['pingou', 'foo'])) if __name__ == '__main__': diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 49c5f6c..6432e7f 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2877,86 +2877,6 @@ class PagureLibtests(tests.Modeltests): html = pagure.lib.text2markdown(text) self.assertEqual(html, expected[idx]) - def test_is_watching_obj(self): - """ Test the is_watching_obj method in pagure.lib """ - # Create the project ns/test - item = pagure.lib.model.Project( - user_id=1, # pingou - name='test3', - namespace='ns', - description='test project #1', - hook_token='aaabbbcccdd', - ) - item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] - self.session.add(item) - self.session.commit() - - # Create the ticket - iss = pagure.lib.new_issue( - issue_id=4, - session=self.session, - repo=item, - title='test issue', - content='content test issue', - user='pingou', - ticketfolder=None, - ) - self.session.commit() - self.assertEqual(iss.id, 4) - self.assertEqual(iss.title, 'test issue') - - # Created the ticket - self.assertTrue(pagure.lib.is_watching_obj( - self.session, 'pingou', iss)) - self.assertFalse(pagure.lib.is_watching_obj( - self.session, 'foo', iss)) - self.assertFalse(pagure.lib.is_watching_obj( - self.session, 'bar', iss)) - - # Comment on the ticket - out = pagure.lib.add_issue_comment( - self.session, - issue=iss, - comment='This is a comment', - user='foo', - ticketfolder=None, - notify=False) - self.assertEqual(out, 'Comment added') - - # Commented on the ticket - self.assertTrue(pagure.lib.is_watching_obj( - self.session, 'pingou', iss)) - self.assertTrue(pagure.lib.is_watching_obj( - self.session, 'foo', iss)) - self.assertFalse(pagure.lib.is_watching_obj( - self.session, 'bar', iss)) - - # Add user `bar` - item = pagure.lib.model.User( - user='bar', - fullname='bar name', - password='bar', - default_email='bar@bar.com', - ) - self.session.add(item) - item = pagure.lib.model.UserEmail( - user_id=3, - email='bar@bar.com') - self.session.add(item) - self.session.commit() - - # Watch the ticket - out = pagure.lib.set_watch_obj(self.session, 'bar', iss, True) - self.assertEqual(out, 'You are now watching this issue') - - # Is watching the ticket - self.assertTrue(pagure.lib.is_watching_obj( - self.session, 'pingou', iss)) - self.assertTrue(pagure.lib.is_watching_obj( - self.session, 'foo', iss)) - self.assertTrue(pagure.lib.is_watching_obj( - self.session, 'bar', iss)) - def test_set_watch_obj(self): """ Test the set_watch_obj method in pagure.lib """ # Create the project ns/test From fc049c2b42fb35792122e2116b7a71dd35a13fa5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 02 2016 09:45:58 +0000 Subject: [PATCH 6/6] Add unit-tests for pagure.lib.get_watch_list --- diff --git a/tests/test_pagure_lib_watch_list.py b/tests/test_pagure_lib_watch_list.py new file mode 100644 index 0000000..40cebd4 --- /dev/null +++ b/tests/test_pagure_lib_watch_list.py @@ -0,0 +1,462 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2016 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import unittest +import shutil +import sys +import os + +import mock + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.lib +import pagure.lib.model +import tests + +@mock.patch( + 'pagure.lib.git.update_git', mock.MagicMock(return_value=True)) +@mock.patch( + 'pagure.lib.notify.send_email', mock.MagicMock(return_value=True)) +class PagureLibGetWatchListtests(tests.Modeltests): + """ Tests for pagure.lib.get_watch_list """ + + def test_get_watch_list_invalid_object(self): + """ Test get_watch_list when given an invalid object """ + # Create a project ns/test + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + namespace='ns', + description='test project #1', + hook_token='aaabbbcccdd', + ) + item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] + self.session.add(item) + self.session.commit() + + self.assertRaises( + pagure.exceptions.InvalidObjectException, + pagure.lib.get_watch_list, + self.session, + item + ) + + def test_get_watch_list_simple(self): + """ Test get_watch_list when the creator of the ticket is the + creator of the project """ + # Create a project ns/test + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + namespace='ns', + description='test project #1', + hook_token='aaabbbcccdd', + ) + item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] + self.session.add(item) + self.session.commit() + + # Create the ticket + iss = pagure.lib.new_issue( + issue_id=4, + session=self.session, + repo=item, + title='test issue', + content='content test issue', + user='pingou', + ticketfolder=None, + ) + self.session.commit() + self.assertEqual(iss.id, 4) + self.assertEqual(iss.title, 'test issue') + + self.assertEqual( + pagure.lib.get_watch_list(self.session, iss), + set(['pingou']) + ) + + def test_get_watch_list_different_creator(self): + """ Test get_watch_list when the creator of the ticket is not the + creator of the project """ + # Create a project ns/test + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + namespace='ns', + description='test project #1', + hook_token='aaabbbcccdd', + ) + item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] + self.session.add(item) + self.session.commit() + + # Create the ticket + iss = pagure.lib.new_issue( + issue_id=4, + session=self.session, + repo=item, + title='test issue', + content='content test issue', + user='foo', + ticketfolder=None, + ) + self.session.commit() + self.assertEqual(iss.id, 4) + self.assertEqual(iss.title, 'test issue') + + self.assertEqual( + pagure.lib.get_watch_list(self.session, iss), + set(['pingou', 'foo']) + ) + + def test_get_watch_list_project_w_contributor(self): + """ Test get_watch_list when the project has more than one + contributor """ + # Create a project ns/test3 + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + namespace='ns', + description='test project #1', + hook_token='aaabbbcccdd', + ) + item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] + self.session.add(item) + self.session.commit() + + # Add a contributor to the project + item = pagure.lib.model.User( + user='bar', + fullname='bar foo', + password='foo', + default_email='bar@bar.com', + ) + self.session.add(item) + item = pagure.lib.model.UserEmail( + user_id=3, + email='bar@bar.com') + self.session.add(item) + + project = pagure.lib.get_project( + self.session, 'test3', namespace='ns') + msg = pagure.lib.add_user_to_project( + session=self.session, + project=project, + new_user='bar', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'User added') + + # Create the ticket + iss = pagure.lib.new_issue( + issue_id=4, + session=self.session, + repo=project, + title='test issue', + content='content test issue', + user='foo', + ticketfolder=None, + ) + self.session.commit() + self.assertEqual(iss.id, 4) + self.assertEqual(iss.title, 'test issue') + + self.assertEqual( + pagure.lib.get_watch_list(self.session, iss), + set(['pingou', 'foo', 'bar']) + ) + + def test_get_watch_list_user_in_group(self): + """ Test get_watch_list when the project has groups of contributors + """ + # Create a project ns/test3 + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + namespace='ns', + description='test project #1', + hook_token='aaabbbcccdd', + ) + item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] + self.session.add(item) + self.session.commit() + + # Create a third user + item = pagure.lib.model.User( + user='bar', + fullname='bar foo', + password='foo', + default_email='bar@bar.com', + ) + self.session.add(item) + item = pagure.lib.model.UserEmail( + user_id=3, + email='bar@bar.com') + self.session.add(item) + + # Create a group + msg = pagure.lib.add_group( + self.session, + group_name='foo', + display_name='foo group', + description=None, + group_type='bar', + user='pingou', + is_admin=False, + blacklist=[], + ) + self.session.commit() + self.assertEqual(msg, 'User `pingou` added to the group `foo`.') + + # Add user to group + group = pagure.lib.search_groups(self.session, group_name='foo') + msg = pagure.lib.add_user_to_group( + self.session, + username='bar', + group=group, + user='pingou', + is_admin=False, + ) + self.session.commit() + self.assertEqual(msg, 'User `bar` added to the group `foo`.') + + project = pagure.lib.get_project( + self.session, 'test3', namespace='ns') + + # Add group to project + msg = pagure.lib.add_group_to_project( + session=self.session, + project=project, + new_group='foo', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'Group added') + + # Create the ticket + iss = pagure.lib.new_issue( + issue_id=4, + session=self.session, + repo=project, + title='test issue', + content='content test issue', + user='foo', + ticketfolder=None, + ) + self.session.commit() + self.assertEqual(iss.id, 4) + self.assertEqual(iss.title, 'test issue') + + self.assertEqual( + pagure.lib.get_watch_list(self.session, iss), + set(['pingou', 'foo', 'bar']) + ) + + def test_get_watch_list_project_w_contributor_out(self): + """ Test get_watch_list when the project has one contributor not + watching the project """ + # Create a project ns/test3 + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + namespace='ns', + description='test project #1', + hook_token='aaabbbcccdd', + ) + item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] + self.session.add(item) + self.session.commit() + + # Add a contributor to the project + item = pagure.lib.model.User( + user='bar', + fullname='bar foo', + password='foo', + default_email='bar@bar.com', + ) + self.session.add(item) + item = pagure.lib.model.UserEmail( + user_id=3, + email='bar@bar.com') + self.session.add(item) + + project = pagure.lib.get_project( + self.session, 'test3', namespace='ns') + msg = pagure.lib.add_user_to_project( + session=self.session, + project=project, + new_user='bar', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'User added') + + # Set the user `pingou` to not watch the project + msg = pagure.lib.update_watch_status( + session=self.session, + project=project, + user='pingou', + watch=False, + ) + self.session.commit() + self.assertEqual(msg, 'You are no longer watching this repo.') + + # Create the ticket + iss = pagure.lib.new_issue( + issue_id=4, + session=self.session, + repo=project, + title='test issue', + content='content test issue', + user='foo', + ticketfolder=None, + ) + self.session.commit() + self.assertEqual(iss.id, 4) + self.assertEqual(iss.title, 'test issue') + + self.assertEqual( + pagure.lib.get_watch_list(self.session, iss), + set(['foo', 'bar']) + ) + + def test_get_watch_list_project_w_contributor_out_pr(self): + """ Test get_watch_list when the project has one contributor not + watching the pull-request """ + # Create a project ns/test3 + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + namespace='ns', + description='test project #1', + hook_token='aaabbbcccdd', + ) + item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] + self.session.add(item) + self.session.commit() + + # Add a contributor to the project + item = pagure.lib.model.User( + user='bar', + fullname='bar foo', + password='foo', + default_email='bar@bar.com', + ) + self.session.add(item) + item = pagure.lib.model.UserEmail( + user_id=3, + email='bar@bar.com') + self.session.add(item) + + project = pagure.lib.get_project( + self.session, 'test3', namespace='ns') + msg = pagure.lib.add_user_to_project( + session=self.session, + project=project, + new_user='bar', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'User added') + + # Create the pull-request + req = pagure.lib.new_pull_request( + session=self.session, + repo_from=project, + branch_from='dev', + repo_to=project, + branch_to='master', + title='test pull-request', + user='foo', + requestfolder=None, + ) + self.session.commit() + self.assertEqual(req.id, 1) + self.assertEqual(req.title, 'test pull-request') + + # Set the user `pingou` to not watch the pull-request + out = pagure.lib.set_watch_obj(self.session, 'pingou', req, False) + self.assertEqual( + out, 'You are no longer watching this pull-request') + + self.assertEqual( + pagure.lib.get_watch_list(self.session, req), + set(['foo', 'bar']) + ) + + def test_get_watch_list_project_w_contributor_watching_project(self): + """ Test get_watch_list when the project has one contributor watching + the project """ + # Create a project ns/test3 + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test3', + namespace='ns', + description='test project #1', + hook_token='aaabbbcccdd', + ) + item.close_status = ['Invalid', 'Insufficient data', 'Fixed'] + self.session.add(item) + self.session.commit() + + # Add a new user + item = pagure.lib.model.User( + user='bar', + fullname='bar foo', + password='foo', + default_email='bar@bar.com', + ) + self.session.add(item) + item = pagure.lib.model.UserEmail( + user_id=3, + email='bar@bar.com') + self.session.add(item) + + # Set the user `bar` to watch the project + project = pagure.lib.get_project( + self.session, 'test3', namespace='ns') + msg = pagure.lib.update_watch_status( + session=self.session, + project=project, + user='bar', + watch=True, + ) + self.session.commit() + self.assertEqual(msg, 'You are now watching this repo.') + + # Create the pull-request + req = pagure.lib.new_pull_request( + session=self.session, + repo_from=project, + branch_from='dev', + repo_to=project, + branch_to='master', + title='test pull-request', + user='foo', + requestfolder=None, + ) + self.session.commit() + self.assertEqual(req.id, 1) + self.assertEqual(req.title, 'test pull-request') + + self.assertEqual( + pagure.lib.get_watch_list(self.session, req), + set(['foo', 'bar', 'pingou']) + ) + + +if __name__ == '__main__': + unittest.main()