From 784502195d27b5018d0e22a4ca4d71e32a53ae6b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 15 2016 13:25:43 +0000 Subject: [PATCH 1/6] Adjust is_watching - Keep user be the user passed on to the method - Make sure the method never raises an exception, this method is called in every single endpoint of the repo controller making it painful if it return True/False and raises exception while at the same time not working if the user is logged out. --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 340fd3f..d72a3c8 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -2784,17 +2784,16 @@ def update_watch_status(session, project, user, watch): msg_success = 'You are no longer watching this repo.' return msg_success + def is_watching(session, user, project): ''' Check user watching the project. ''' if user is None: return False - user = user.username - user_obj = __get_user(session, user) + user_obj = search_user(session, username=user.username) if not user_obj: - raise pagure.exceptions.PagureException( - 'No user with username: %s' % user) + return False watcher = session.query( model.Watcher @@ -2809,12 +2808,12 @@ def is_watching(session, user, project): return watcher.watch watch=False - if user == project.user.user: + if user.username == project.user.username: return True for group in project.groups: for guser in group.users: - if user == guser.username: + if user.username == guser.username: watch=True break From c60175747acedc8831238895111787268868b08a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 15 2016 13:25:43 +0000 Subject: [PATCH 2/6] Adjust the unit-tests for the change to is_watching --- diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 7d624ee..75b41b6 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2257,7 +2257,7 @@ class PagureLibtests(tests.Modeltests): watch='1', ) self.session.commit() - self.assertEqual(msg, 'From now you are watching this repo.') + self.assertEqual(msg, 'You are now watching this repo.') # All good and when user selected unwatch option. msg = pagure.lib.update_watch_status( @@ -2287,13 +2287,12 @@ class PagureLibtests(tests.Modeltests): # User does not exist user = tests.FakeUser() user.username = 'aavrug' - self.assertRaises( - pagure.exceptions.PagureException, - pagure.lib.is_watching, + watch = pagure.lib.is_watching( session=self.session, user=user, project=project, ) + self.assertFalse(watch) pagure.lib.add_group_to_project( session=self.session, @@ -2304,6 +2303,15 @@ class PagureLibtests(tests.Modeltests): self.session.commit() group = pagure.lib.search_groups(self.session, group_name='foo') + pagure.lib.set_up_user( + session=self.session, + username='foo', + fullname='foo bar', + default_email='foo@bar.com', + ssh_key=None, + keydir=None, + ) + self.session.commit() pagure.lib.add_user_to_group( self.session, username='foo', @@ -2312,6 +2320,7 @@ class PagureLibtests(tests.Modeltests): is_admin=False, ) self.session.commit() + group = pagure.lib.search_groups(self.session, group_name='foo') # If user belongs to any group of that project user.username = 'foo' From 0fcf17f51b43a0e8cdaf0887e79ad75435ed58ab Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 15 2016 13:25:43 +0000 Subject: [PATCH 3/6] Only check the watch status if the user is authenticated Otherwise flask.g may not have a `fas_user` attribute and we end up with a 500 error. --- diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 5a17114..52fd809 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -39,7 +39,7 @@ import pagure.forms import pagure import pagure.ui.plugins from pagure import (APP, SESSION, LOG, __get_file_in_tree, login_required, - is_repo_admin, admin_session_timedout) + is_repo_admin, admin_session_timedout, authenticated) # pylint: disable=E1101 @@ -130,6 +130,10 @@ def view_repo(repo, username=None): break diff_commits.append(commit.oid.hex) + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'repo_info.html', select='overview', @@ -147,7 +151,7 @@ def view_repo(repo, username=None): diff_commits=diff_commits, repo_admin=is_repo_admin(repo), form=pagure.forms.ConfirmationForm(), - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -232,6 +236,10 @@ def view_repo_branch(repo, branchname, username=None): 'view_raw_file', username=username, repo=repo.name, identifier=branchname, filename='')) + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'repo_info.html', select='overview', @@ -248,7 +256,7 @@ def view_repo_branch(repo, branchname, username=None): diff_commits=diff_commits, repo_admin=is_repo_admin(repo), form=pagure.forms.ConfirmationForm(), - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -345,6 +353,10 @@ def view_commits(repo, branchname=None, username=None): diff_commits.append(commit.oid.hex) diff_commits_full.append(commit) + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'commits.html', select='commits', @@ -363,7 +375,7 @@ def view_commits(repo, branchname=None, username=None): total_page=total_page, repo_admin=is_repo_admin(repo), form=pagure.forms.ConfirmationForm(), - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -461,6 +473,10 @@ def view_file(repo, identifier, filename, username=None): if encoding: headers['Content-Encoding'] = encoding + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return ( flask.render_template( 'file.html', @@ -474,7 +490,7 @@ def view_file(repo, identifier, filename, username=None): content=content, output_type=output_type, repo_admin=is_repo_admin(repo), - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ), 200, headers @@ -626,6 +642,10 @@ def view_commit(repo, commitid, username=None): # First commit in the repo diff = commit.tree.diff_to_tree(swap=True) + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'commit.html', select='commits', @@ -636,7 +656,7 @@ def view_commit(repo, commitid, username=None): commitid=commitid, commit=commit, diff=diff, - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -712,6 +732,10 @@ def view_tree(repo, identifier=None, username=None): content = sorted(commit.tree, key=lambda x: x.filemode) output_type = 'tree' + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'file.html', select='tree', @@ -725,7 +749,7 @@ def view_tree(repo, identifier=None, username=None): content=content, output_type=output_type, repo_admin=is_repo_admin(repo), - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -741,13 +765,17 @@ def view_forks(repo, username=None): if not repo: flask.abort(404, 'Project not found') + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'forks.html', select='forks', username=username, repo=repo, repo_admin=is_repo_admin(repo), - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -767,6 +795,11 @@ def view_tags(repo, username=None): repo_obj = pygit2.Repository(reponame) tags = pagure.lib.git.get_git_tags_objects(repo) + + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'releases.html', select='tags', @@ -775,7 +808,7 @@ def view_tags(repo, username=None): tags=tags, repo_admin=is_repo_admin(repo), repo_obj=repo_obj, - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -820,13 +853,17 @@ def new_release(repo, username=None): return flask.redirect( flask.url_for('view_tags', repo=repo.name, username=username)) + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'new_release.html', select='tags', username=username, repo=repo, form=form, - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -900,6 +937,10 @@ def view_settings(repo, username=None): if flask.request.method == 'GET' and branchname: branches_form.branches.data = branchname + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'settings.html', select='settings', @@ -913,7 +954,7 @@ def view_settings(repo, username=None): plugins=plugins, repo_admin=repo_admin, branchname = branchname, - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -1388,11 +1429,16 @@ def add_user(repo, username=None): APP.logger.exception(err) flask.flash('User could not be added', 'error') + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'add_user.html', form=form, username=username, repo=repo, + watch=watch, ) @@ -1505,12 +1551,16 @@ def add_group_project(repo, username=None): APP.logger.exception(err) flask.flash('Group could not be added', 'error') + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'add_group_project.html', form=form, username=username, repo=repo, - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -1609,6 +1659,10 @@ def add_token(repo, username=None): APP.logger.exception(err) flask.flash('User could not be added', 'error') + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'add_token.html', select='settings', @@ -1617,7 +1671,7 @@ def add_token(repo, username=None): repo_admin=repo_admin, username=username, repo=repo, - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -1749,6 +1803,10 @@ def edit_file(repo, branchname, filename, username=None): else: data = form.content.data.decode('utf-8') + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'edit_file.html', select='tree', @@ -1760,7 +1818,7 @@ def edit_file(repo, branchname, filename, username=None): form=form, user=user, branches=repo_obj.listall_branches(), - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo), + watch=watch, ) @@ -1817,6 +1875,10 @@ def view_docs(repo, username=None, filename=None): if not APP.config.get('DOC_APP_URL'): flask.abort(404, 'This pagure instance has no doc server') + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'docs.html', select='docs', @@ -1824,9 +1886,10 @@ def view_docs(repo, username=None, filename=None): username=username, filename=filename, endpoint='view_docs', - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo_obj), + watch=watch2, ) + @APP.route('//activity/') @APP.route('//activity') def view_project_activity(repo): @@ -1841,11 +1904,17 @@ def view_project_activity(repo): if not repo_obj: flask.abort(404, 'Project not found') + watch = False + if authenticated(): + watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + return flask.render_template( 'activity.html', repo=repo_obj, + watch=watch, ) + @APP.route('/watch//settings/', methods=['POST']) @APP.route('/watch/fork///settings/', methods=['POST']) @login_required From fbfefeeb1caedc41ef774dc2cbbe69f963abaea8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 15 2016 13:25:43 +0000 Subject: [PATCH 4/6] Remove a chunk of code that's not needed --- diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 75b41b6..8b3d050 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2303,15 +2303,6 @@ class PagureLibtests(tests.Modeltests): self.session.commit() group = pagure.lib.search_groups(self.session, group_name='foo') - pagure.lib.set_up_user( - session=self.session, - username='foo', - fullname='foo bar', - default_email='foo@bar.com', - ssh_key=None, - keydir=None, - ) - self.session.commit() pagure.lib.add_user_to_group( self.session, username='foo', From cc69f7f36ad6abb83c5316a8932706aca0110c18 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 15 2016 13:27:34 +0000 Subject: [PATCH 5/6] Simplify the end of the is_watching method as @sayanchowdhury suggested --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index d72a3c8..2a60815 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -2806,15 +2806,12 @@ def is_watching(session, user, project): if watcher: return watcher.watch - - watch=False if user.username == project.user.username: return True for group in project.groups: for guser in group.users: if user.username == guser.username: - watch=True - break + return True - return watch + return False From ca6da27bf5623b3292b1097e5405a6eeba98fcd5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 15 2016 13:27:57 +0000 Subject: [PATCH 6/6] Fix typo watch2->watch and pep8 with assigning the watch variable --- diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 52fd809..c668258 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -132,7 +132,7 @@ def view_repo(repo, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'repo_info.html', @@ -238,7 +238,7 @@ def view_repo_branch(repo, branchname, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'repo_info.html', @@ -355,7 +355,7 @@ def view_commits(repo, branchname=None, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'commits.html', @@ -475,7 +475,7 @@ def view_file(repo, identifier, filename, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return ( flask.render_template( @@ -644,7 +644,7 @@ def view_commit(repo, commitid, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'commit.html', @@ -734,7 +734,7 @@ def view_tree(repo, identifier=None, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'file.html', @@ -767,7 +767,7 @@ def view_forks(repo, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'forks.html', @@ -798,7 +798,7 @@ def view_tags(repo, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'releases.html', @@ -855,7 +855,7 @@ def new_release(repo, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'new_release.html', @@ -939,7 +939,7 @@ def view_settings(repo, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'settings.html', @@ -1431,7 +1431,7 @@ def add_user(repo, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'add_user.html', @@ -1553,7 +1553,7 @@ def add_group_project(repo, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'add_group_project.html', @@ -1661,7 +1661,7 @@ def add_token(repo, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'add_token.html', @@ -1805,7 +1805,7 @@ def edit_file(repo, branchname, filename, username=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'edit_file.html', @@ -1877,7 +1877,7 @@ def view_docs(repo, username=None, filename=None): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'docs.html', @@ -1886,7 +1886,7 @@ def view_docs(repo, username=None, filename=None): username=username, filename=filename, endpoint='view_docs', - watch=watch2, + watch=watch, ) @@ -1906,7 +1906,7 @@ def view_project_activity(repo): watch = False if authenticated(): - watch=pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) + watch = pagure.lib.is_watching(SESSION, flask.g.fas_user, repo) return flask.render_template( 'activity.html',