From f7a1bc1cd94874f920884ed835141930876b2a42 Mon Sep 17 00:00:00 2001 From: Michal Konečný Date: Aug 20 2020 12:45:15 +0000 Subject: Add reason for orphaning package This PR implements https://pagure.io/pagure-dist-git/issue/112 and adds following: * New API call to retrieve orphan status with reason * New Form for orphan reason * New table to record the reason * Removal of the reason when package is adopted Signed-off-by: Michal Konečný --- diff --git a/createdb.py b/createdb.py index 5a17246..1bbfdf9 100644 --- a/createdb.py +++ b/createdb.py @@ -29,7 +29,7 @@ if args.config: os.environ["PAGURE_CONFIG"] = config import pagure.config -from pagure_distgit.model import BASE, PagureAnitya, PagureBZOverride +from pagure_distgit.model import BASE, PagureAnitya, PagureBZOverride, PagureOrphanReason db_url = pagure.config.config.get("DB_URL") if db_url.startswith("postgres"): @@ -38,5 +38,9 @@ else: engine = create_engine(db_url, echo=True) BASE.metadata.create_all( - engine, tables=[PagureAnitya.__table__, PagureBZOverride.__table__] + engine, tables=[ + PagureAnitya.__table__, + PagureBZOverride.__table__, + PagureOrphanReason.__table__ + ] ) diff --git a/pagure_distgit/forms.py b/pagure_distgit/forms.py index 6d59a56..4bdf1fe 100644 --- a/pagure_distgit/forms.py +++ b/pagure_distgit/forms.py @@ -30,6 +30,28 @@ class AnityaForm(pagure.forms.PagureForm): ) +class OrphanReasonForm(pagure.forms.PagureForm): + """ Form for orphaning reason. """ + + orphan_reason = wtforms.SelectField( + "Reason for orphaning package", + [wtforms.validators.DataRequired()], + choices=[ + ("Lack of time", "Lack of time"), + ("Do not use it anymore", "Do not use it anymore"), + ("Unmaintained upstream", "Unmaintained upstream"), + ("Fails to build from source", "Fails to build from source"), + ("Important bug not fixed", "Important bug not fixed"), + ("Other", "Other"), + ], + ) + + orphan_reason_info = wtforms.StringField( + "Additional info", + [wtforms.validators.optional(strip_whitespace=True)], + ) + + class BZOverrideForm(pagure.forms.PagureForm): """ Form to configure the Fedora/EPEL maintainer for a project. """ diff --git a/pagure_distgit/model.py b/pagure_distgit/model.py index 4505341..00faf8d 100644 --- a/pagure_distgit/model.py +++ b/pagure_distgit/model.py @@ -57,6 +57,44 @@ class PagureAnitya(BASE): ) +class PagureOrphanReason(BASE): + """ Stores information about the orphaning reason. + + Table -- pagure_orphan_reason + + Attributes: + project_id: Id of orphaned project + reason: Reason for orphaning + reason: Any additional info for orphaning + """ + + __tablename__ = "pagure_orphan_reason" + + id = sa.Column(sa.Integer, primary_key=True) + project_id = sa.Column( + sa.Integer, + sa.ForeignKey("projects.id", onupdate="CASCADE", ondelete="CASCADE"), + nullable=False, + unique=True, + index=True, + ) + + reason = sa.Column(sa.String(255), nullable=True, unique=False) + + reason_info = sa.Column(sa.String(255), nullable=True, unique=False) + + project = relation( + "Project", + remote_side=[Project.id], + backref=backref( + "orphan_reason", + cascade="delete, delete-orphan", + single_parent=True, + uselist=False, + ), + ) + + class PagureBZOverride(BASE): """ Stores information about default assignees (Fedora vs. EPEL) diff --git a/pagure_distgit/plugin.py b/pagure_distgit/plugin.py index ea4f2b4..91aeac0 100644 --- a/pagure_distgit/plugin.py +++ b/pagure_distgit/plugin.py @@ -183,11 +183,77 @@ def _is_active_in_pdc(name, namespace): return data["results"][0]["active"] is True +@DISTGIT_NS.route("/orphan//", methods=["GET"]) +@api_method +def orphan_get_endpoint(namespace, repo): + """ + Orphan state + ------------ + Returns the current orphan state of package with reason. + + :: + + GET /_dg/orphan// + + Sample response + ^^^^^^^^^^^^^^^ + + :: + + { + "orphan": true, + "reason": "Lack of Time", + "reason_info": "Personal issues" + } + """ + repo = _get_repo(repo, namespace=namespace) + + output = {"orphan": False, "reason": "", "reason_info": ""} + + if repo.user.user == "orphan": + output["orphan"] = True + if repo.orphan_reason: + output["reason"] = repo.orphan_reason.reason + output["reason_info"] = repo.orphan_reason.reason_info + return flask.jsonify(output) + + @DISTGIT_NS.route("/orphan//", methods=["POST"]) @api_login_required(acls=["modify_project"]) @api_method def orphan_endpoint(namespace, repo): - """ Orphan the package. + """ + Orphan + ------ + Orphan the package. + + :: + + POST /_dg/orphan// + + Input + ^^^^^ + + +-----------------------+---------+--------------+---------------------------+ + | Key | Type | Optionality | Description | + +=======================+=========+==============+===========================+ + | ``orphan_reason`` | string | Mandatory | | The reason to orphan | + | | | | the package. | + +-----------------------+---------+--------------+---------------------------+ + | ``orphan_reason_info``| string | Optional | | Additional info for | + | | | | provided reason. | + +-----------------------+---------+--------------+---------------------------+ + + Sample response + ^^^^^^^^^^^^^^^ + + :: + + { + "orphan": true, + "reason": "Other", + "reason_info": "Voices in my head tell me to stop" + } """ _log.info("Received a request to orphan: %s/%s", namespace, repo) @@ -211,32 +277,47 @@ def orphan_endpoint(namespace, repo): _log.exception("Error when retrieving orphan user") raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR) - try: - pagure.lib.query.set_project_owner( - flask.g.session, repo, orphan_user_obj - ) - if user_obj in repo.users: - pagure.lib.query.remove_user_of_project( - flask.g.session, user_obj, repo, user_obj.user + form = pagure_distgit.forms.OrphanReasonForm(csrf_enabled=False) + if form.validate_on_submit(): + try: + reason = model.PagureOrphanReason( + project_id=repo.id, + reason=form.orphan_reason.data, + reason_info=form.orphan_reason_info.data, ) - pagure.lib.query.update_watch_status( - flask.g.session, repo, user_obj.username, "-1" + flask.g.session.add(reason) + pagure.lib.query.set_project_owner( + flask.g.session, repo, orphan_user_obj + ) + if user_obj in repo.users: + pagure.lib.query.remove_user_of_project( + flask.g.session, user_obj, repo, user_obj.user + ) + pagure.lib.query.update_watch_status( + flask.g.session, repo, user_obj.username, "-1" + ) + flask.g.session.commit() + except SQLAlchemyError: # pragma: no cover + flask.g.session.rollback() + _log.exception("Error when orphaning project") + raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR) + else: + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EINVALIDREQ, errors=form.errors ) - flask.g.session.commit() - except SQLAlchemyError: # pragma: no cover - flask.g.session.rollback() - _log.exception("Error when orphaning project") - raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR) pagure.lib.notify.log( repo, topic="project.orphan", - msg={"project": repo.to_json(public=True), "agent": user_obj.username}, + msg={ + "project": repo.to_json(public=True), + "agent": user_obj.username, + "reason": repo.orphan_reason.reason, + "reason_info": repo.orphan_reason.reason_info, + }, ) - output = {"point_of_contact": repo.user.user} - - return flask.jsonify(output) + return orphan_get_endpoint(namespace, repo.name) @DISTGIT_NS.route("/take_orphan//", methods=["POST"]) @@ -281,6 +362,9 @@ def take_orphan_endpoint(namespace, repo): try: repo.user = user_obj + if repo.orphan_reason: + reason = repo.orphan_reason + flask.g.session.delete(reason) flask.g.session.add(repo) flask.g.session.commit() except SQLAlchemyError as err: # pragma: no cover diff --git a/pagure_distgit_tests/test_plugin.py b/pagure_distgit_tests/test_plugin.py index aafed1c..0924957 100644 --- a/pagure_distgit_tests/test_plugin.py +++ b/pagure_distgit_tests/test_plugin.py @@ -1,17 +1,18 @@ import json import os -from unittest.mock import call, patch +from unittest.mock import patch -import pagure.lib.query import pagure.lib.model +import pagure.lib.query +from pagure_distgit import model from pagure_distgit import plugin import tests class PagureFlaskApiOrphanEndpointTests(tests.Modeltests): - """ Tests the orphan endpoints added in pagure-dist-git. """ + """ Tests the orphan endpoint added in pagure-dist-git. """ def setUp(self): """ Set up the environnment, ran before every tests. """ @@ -52,6 +53,16 @@ class PagureFlaskApiOrphanEndpointTests(tests.Modeltests): ) assert output.status_code == 401 + def test_invalid_form(self): + """Assert that invalid form is not accepted. + """ + headers = {"Authorization": "token aaabbbcccddd"} + datainput = {} + output = self.app.post( + "/_dg/orphan/somenamespace/test3", data=datainput, headers=headers, + ) + assert output.status_code == 400 + def test_already_orphaned_package(self): """Assert that already orphaned package can't be orphaned again. """ @@ -74,7 +85,7 @@ class PagureFlaskApiOrphanEndpointTests(tests.Modeltests): """Assert that package is correctly orphaned. """ headers = {"Authorization": "token aaabbbcccddd"} - datainput = {} + datainput = {"orphan_reason": "Lack of time"} repo = pagure.lib.query.get_authorized_project( self.session, "test3", namespace="somenamespace", ) @@ -89,7 +100,8 @@ class PagureFlaskApiOrphanEndpointTests(tests.Modeltests): ) assert output.status_code == 200 data = json.loads(output.get_data(as_text=True)) - self.assertDictEqual(data, {"point_of_contact": "orphan"}) + exp = {"orphan": True, "reason": "Lack of time", "reason_info": ""} + self.assertDictEqual(data, exp) assert repo.user.user == "orphan" @@ -107,7 +119,7 @@ class PagureFlaskApiOrphanEndpointTests(tests.Modeltests): """Assert that package is correctly orphaned. """ headers = {"Authorization": "token aaabbbcccddd"} - datainput = {} + datainput = {"orphan_reason": "Lack of time"} repo = pagure.lib.query.get_authorized_project( self.session, "test3", namespace="somenamespace", ) @@ -117,7 +129,8 @@ class PagureFlaskApiOrphanEndpointTests(tests.Modeltests): ) assert output.status_code == 200 data = json.loads(output.get_data(as_text=True)) - self.assertDictEqual(data, {"point_of_contact": "orphan"}) + exp = {"orphan": True, "reason": "Lack of time", "reason_info": ""} + self.assertDictEqual(data, exp) # refresh the repo object, so we have the current state self.session.refresh(repo) @@ -143,3 +156,199 @@ class PagureFlaskApiOrphanEndpointTests(tests.Modeltests): "/_dg/orphan/somenamespace/test3", data=datainput, headers=headers, ) assert output.status_code == 400 + + +class PagureFlaskApiOrphanGetEndpointTests(tests.Modeltests): + """ Tests the orphan get endpoint added in pagure-dist-git. """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskApiOrphanGetEndpointTests, self).setUp() + self.session.flush() + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, "repos"), bare=True) + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + tests.create_user( + self.session, "orphan", "orphan", ["orphan@fedoraproject.org"] + ) + repo = pagure.lib.query.get_authorized_project( + self.session, "test3", namespace="somenamespace", + ) + token = pagure.lib.query.get_api_token(self.session, "aaabbbcccddd") + token.project = repo + self.session.add(token) + self.session.commit() + self._app.register_blueprint(plugin.DISTGIT_NS) + + def test_not_orphaned(self): + """ + Assert that correct output is returned when package is not orphaned. + """ + output = self.app.get("/_dg/orphan/somenamespace/test3") + assert output.status_code == 200 + + data = json.loads(output.get_data(as_text=True)) + exp = {"orphan": False, "reason": "", "reason_info": ""} + self.assertDictEqual(data, exp) + + def test_orphaned(self): + """ Assert that correct output is returned when package is orphaned. + """ + orphan_user_obj = pagure.lib.query.get_user(self.session, "orphan") + repo = pagure.lib.query.get_authorized_project( + self.session, "test3", namespace="somenamespace", + ) + repo.user = orphan_user_obj + reason = model.PagureOrphanReason( + project_id=repo.id, reason="reason", reason_info="reason_info", + ) + self.session.add(reason) + self.session.add(repo) + self.session.commit() + output = self.app.get("/_dg/orphan/somenamespace/test3") + assert output.status_code == 200 + + data = json.loads(output.get_data(as_text=True)) + exp = { + "orphan": True, + "reason": "reason", + "reason_info": "reason_info", + } + self.assertDictEqual(data, exp) + + def test_orphaned_no_reason(self): + """ Assert that correct output is returned when package is orphaned. + """ + orphan_user_obj = pagure.lib.query.get_user(self.session, "orphan") + repo = pagure.lib.query.get_authorized_project( + self.session, "test3", namespace="somenamespace", + ) + repo.user = orphan_user_obj + self.session.add(repo) + self.session.commit() + output = self.app.get("/_dg/orphan/somenamespace/test3") + assert output.status_code == 200 + + data = json.loads(output.get_data(as_text=True)) + exp = { + "orphan": True, + "reason": "", + "reason_info": "", + } + self.assertDictEqual(data, exp) + + +class PagureFlaskApiTakeOrphanEndpointTests(tests.Modeltests): + """ Tests the take orphan endpoint added in pagure-dist-git. """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskApiTakeOrphanEndpointTests, self).setUp() + self.session.flush() + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, "repos"), bare=True) + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + tests.create_user( + self.session, "orphan", "orphan", ["orphan@fedoraproject.org"] + ) + pagure.lib.query.add_group( + self.session, + "packager", + "packager", + "", + "user", + "pingou", + True, + [], + ) + + repo = pagure.lib.query.get_authorized_project( + self.session, "test3", namespace="somenamespace", + ) + token = pagure.lib.query.get_api_token(self.session, "aaabbbcccddd") + token.project = repo + orphan_user_obj = pagure.lib.query.get_user(self.session, "orphan") + repo.user = orphan_user_obj + reason = model.PagureOrphanReason( + project_id=repo.id, reason="reason", reason_info="reason_info", + ) + self.session.add(reason) + self.session.add(repo) + self.session.add(token) + self.session.commit() + self._app.register_blueprint(plugin.DISTGIT_NS) + + def test_token_missing_ACL(self): + """ + Test the take orphan endpoint with an API token missing + the `modify_project` ACL. + """ + headers = {"Authorization": "token foo_token"} + output = self.app.post( + "/_dg/take_orphan/somenamespace/test3", headers=headers + ) + # invalid token + assert output.status_code == 401 + + def test_invalid_token(self): + """Test the take orphan endpoint with an invalid API token. """ + headers = {"Authorization": "token BBBZZZOOO"} + output = self.app.post( + "/_dg/take_orphan/somenamespace/test3", headers=headers, + ) + assert output.status_code == 401 + + @patch("pagure_distgit.plugin._is_active_in_pdc") + @patch("pagure_distgit.plugin.pagure.lib.notify.log") + def test_take_orphan(self, mock_log, mock_pdc): + """Assert that package is correctly adopted. + """ + mock_pdc.return_value = True + headers = {"Authorization": "token aaabbbcccddd"} + repo = pagure.lib.query.get_authorized_project( + self.session, "test3", namespace="somenamespace", + ) + assert repo.orphan_reason.reason == "reason" + output = self.app.post( + "/_dg/take_orphan/somenamespace/test3", headers=headers, + ) + assert output.status_code == 200 + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual(data, {"point_of_contact": "pingou"}) + + # refresh the repo object, so we have the current state + self.session.refresh(repo) + assert repo.user.user == "pingou" + assert not repo.orphan_reason + + assert mock_log.call_count == 1 + + @patch("pagure_distgit.plugin._is_active_in_pdc") + @patch("pagure_distgit.plugin.pagure.lib.notify.log") + def test_take_orphan_no_reason(self, mock_log, mock_pdc): + """ + Assert that package is correctly adopted when reason + is not provided. + """ + mock_pdc.return_value = True + headers = {"Authorization": "token aaabbbcccddd"} + repo = pagure.lib.query.get_authorized_project( + self.session, "test3", namespace="somenamespace", + ) + reason = repo.orphan_reason + self.session.delete(reason) + self.session.commit() + output = self.app.post( + "/_dg/take_orphan/somenamespace/test3", headers=headers, + ) + assert output.status_code == 200 + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual(data, {"point_of_contact": "pingou"}) + + # refresh the repo object, so we have the current state + self.session.refresh(repo) + assert repo.user.user == "pingou" + + assert mock_log.call_count == 1