PR#4452: [WIP] Add repo_from argument for API create pull request - pagure

pagure

#4452 [WIP] Add repo_from argument for API create pull request

Merged 4 years ago by pingou. Opened 4 years ago by lenkaseg.

Unknown source repo_from into master

Adjust how are passed the arguments about repo_from when opening a new PR

Pierre-Yves Chibon • 4 years ago

6fa4b3d

Code style changes and documentation

Pierre-Yves Chibon • 4 years ago

0c23518

Add repo_from argument for API create pull request

Lenka Segura • 4 years ago

98b37a4

pagure/api/fork.py

file modified

+67 -36

		`@@ -1279,25 +1279,45 @@`
		`Input`
		`^^^^^`

		`- +--------------------+----------+---------------+----------------------+`
		`- \| Key \| Type \| Optionality \| Description \|`
		`- +====================+==========+===============+======================+`
		- \| ``title`` \| string \| Mandatory \| The title to give to \|
		`- \| \| \| \| this pull-request \|`
		`- +--------------------+----------+---------------+----------------------+`
		- \| ``branch_to`` \| string \| Mandatory \| The name of the \|
		`- \| \| \| \| branch the submitted \|`
		`- \| \| \| \| changes should be \|`
		`- \| \| \| \| merged into. \|`
		`- +--------------------+----------+---------------+----------------------+`
		- \| ``branch_from`` \| string \| Mandatory \| The name of the \|
		`- \| \| \| \| branch containing \|`
		`- \| \| \| \| the changes to merge \|`
		`- +--------------------+----------+---------------+----------------------+`
		- \| ``initial_comment``\| string \| Optional \| The intial comment \|
		`- \| \| \| \| describing what these\|`
		`- \| \| \| \| changes are about. \|`
		`- +--------------------+----------+---------------+----------------------+`
		`+ +-----------------------+----------+-------------+------------------------+`
		`+ \| Key \| Type \| Optionality \| Description \|`
		`+ +=======================+==========+=============+========================+`
		+ \| ``title`` \| string \| Mandatory \| The title to give to \|
		`+ \| \| \| \| this pull-request \|`
		`+ +-----------------------+----------+-------------+------------------------+`
		+ \| ``branch_to`` \| string \| Mandatory \| The name of the branch \|
		`+ \| \| \| \| the submitted changes \|`
		`+ \| \| \| \| should be merged into. \|`
		`+ +-----------------------+----------+-------------+------------------------+`
		+ \| ``branch_from`` \| string \| Mandatory \| The name of the branch \|
		`+ \| \| \| \| containing the changes \|`
		`+ \| \| \| \| to merge \|`
		`+ +-----------------------+----------+-------------+------------------------+`
		+ \| ``repo_from`` \| string \| Optional \| The name of the project\|
		`+ \| \| \| \| the changes originate \|`
		`+ \| \| \| \| from. \|`
		`+ \| \| \| \| If not specified the \|`
		`+ \| \| \| \| repo_from is assumed \|`
		`+ \| \| \| \| to be the repo_to. \|`
		`+ +-----------------------+----------+-------------+------------------------+`
		+ \| ``repo_from_username``\| string \| Optional \| The username of the \|
		`+ \| \| \| \| project the changes \|`
		`+ \| \| \| \| originate from. \|`
		`+ \| \| \| \| If not specified the \|`
		`+ \| \| \| \| repo_from is assumed \|`
		`+ \| \| \| \| to be the repo_to. \|`
		`+ +-----------------------+----------+-------------+------------------------+`
		+ \|``repo_from_namespace``\| string \| Optional \| The namespace of the \|
		`+ \| \| \| \| project the changes \|`
		`+ \| \| \| \| originate from. \|`
		`+ \| \| \| \| If not specified the \|`
		`+ \| \| \| \| repo_from is assumed \|`
		`+ \| \| \| \| to be the repo_to. \|`
		`+ +-----------------------+----------+-------------+------------------------+`
		+ \| ``initial_comment`` \| string \| Optional \| The intial comment \|
		`+ \| \| \| \| describing what these \|`
		`+ \| \| \| \| changes are about. \|`
		`+ +-----------------------+----------+-------------+------------------------+`

		`Sample response`
		`^^^^^^^^^^^^^^^`
		`@@ -1349,11 +1369,26 @@`
		`}`
		`}`

		`- """`
		`+ """ # noqa`

		`- repo = _get_repo(repo, username, namespace)`
		`- _check_pull_request(repo)`
		`- _check_token(repo)`
		`+ repo_to = _get_repo(repo, username, namespace)`
		`+`
		`+ req_data = get_request_data()`
		`+ repo_from = req_data.get("repo_from")`
		`+ repo_from_username = req_data.get("repo_from_username")`
		`+ repo_from_namespace = req_data.get("repo_from_namespace")`
		`+`
		`+ if repo_from:`
		`+ repo_from = _get_repo(`
		`+ repo_from,`
		`+ username=repo_from_username,`
		`+ namespace=repo_from_namespace,`
		`+ )`
		`+ else:`
		`+ repo_from = repo_to`
		`+`
		`+ _check_pull_request(repo_to)`
		`+ _check_token(repo_from)`

		`form = pagure.forms.RequestPullForm(csrf_enabled=False)`
		`if not form.validate_on_submit():`
		`@@ -1375,35 +1410,31 @@`
		`errors={"branch_from": ["This field is required."]},`
		`)`

		`- parent = repo`
		`- if repo.parent:`
		`- parent = repo.parent`
		`-`
		`- if not parent.settings.get("pull_requests", True):`
		`+ if not repo_to.settings.get("pull_requests", True):`
		`raise pagure.exceptions.APIError(`
		`404, error_code=APIERROR.EPULLREQUESTSDISABLED`
		`)`

		`- repo_committer = pagure.utils.is_repo_committer(repo)`
		`+ repo_committer = pagure.utils.is_repo_committer(repo_from)`

		`if not repo_committer:`
		`raise pagure.exceptions.APIError(`
		`401, error_code=APIERROR.ENOTHIGHENOUGH`
		`)`

		`- repo_obj = pygit2.Repository(repo.repopath("main"))`
		`- orig_repo = pygit2.Repository(parent.repopath("main"))`
		`+ git_repo_from = pygit2.Repository(repo_from.repopath("main"))`
		`+ git_repo_to = pygit2.Repository(repo_to.repopath("main"))`

		`try:`
		`diff, diff_commits, orig_commit = pagure.lib.git.get_diff_info(`
		`- repo_obj, orig_repo, branch_from, branch_to`
		`+ git_repo_from, git_repo_to, branch_from, branch_to`
		`)`
		`except pagure.exceptions.PagureException as err:`
		`raise pagure.exceptions.APIError(`
		`400, error_code=APIERROR.EINVALIDREQ, errors=str(err)`
		`)`

		`- if parent.settings.get(`
		`+ if repo_to.settings.get(`
		`"Enforce_signed-off_commits_in_pull-request", False`
		`):`
		`for commit in diff_commits:`
		`@@ -1424,10 +1455,10 @@`

		`request = pagure.lib.query.new_pull_request(`
		`flask.g.session,`
		`- repo_to=parent,`
		`+ repo_to=repo_to,`
		`branch_to=branch_to,`
		`branch_from=branch_from,`
		`- repo_from=repo,`
		`+ repo_from=repo_from,`
		`title=form.title.data,`
		`initial_comment=initial_comment,`
		`user=flask.g.fas_user.username,`
		`@@ -1577,7 +1608,7 @@`
		`for patch in diff:`
		`stats = pagure.lib.git.get_stats_patch(patch)`
		`new_path = stats["new_path"]`
		`- del (stats["new_path"])`
		`+ del stats["new_path"]`
		`output[new_path] = stats`
		`else:`
		`raise pagure.exceptions.APIError(400, error_code=APIERROR.ENOPRSTATS)`

pagure/lib/model.py

file modified

+14

		`@@ -470,6 +470,20 @@`
		`viewonly=True,`
		`)`

		`+ def __repr__(self):`
		`+ return (`
		`+ "Project(%s, name:%s, namespace:%s, url:%s, is_fork:%s,\`
		`+ parent_id:%s)"`
		`+ % (`
		`+ self.id,`
		`+ self.name,`
		`+ self.namespace,`
		`+ self.url,`
		`+ self.is_fork,`
		`+ self.parent_id,`
		`+ )`
		`+ )`
		`+`
		`@property`
		`def isa(self):`
		`""" A string to allow finding out that this is a project. """`

tests/test_pagure_flask_api_fork.py

file modified

+3 -3

		`@@ -2810,12 +2810,12 @@`
		`"initial_comment": "the manifest",`
		`"branch_to": "master",`
		`"branch_from": "branch",`
		`+ "repo_from": "test",`
		`+ "repo_from_username": "pingou",`
		`}`

		`output = self.app.post(`
		`- "/api/0/fork/pingou/test/pull-request/new",`
		`- headers=headers,`
		`- data=data,`
		`+ "/api/0/test/pull-request/new", headers=headers, data=data`
		`)`
		`self.assertEqual(output.status_code, 200)`

tests/test_pagure_flask_ui_repo.py

file modified

+1 -3

		`@@ -6757,9 +6757,7 @@`
		`""" Test the test_hook endpoint when the user is logged in. """`
		`user = tests.FakeUser(username="pingou")`
		`with tests.user_set(self.app.application, user):`
		`- data = {`
		`- "csrf_token": self.get_csrf()`
		`- }`
		`+ data = {"csrf_token": self.get_csrf()}`
		`output = self.app.post("/test/settings/test_hook", data=data)`
		`self.assertEqual(output.status_code, 302)`

lenkaseg commented 4 years ago

New argument repo_from added. Now it is possible to open a pull request from fork to a another fork (of the same parent) or to a parent.
The repo_from is expected to be a dictionary, example:

{'username': 'foo', 'namespace': None, 'repo': 'carrot'}

I also added the __repr__ function of a class Project in pagure/lib/model.py. The other classes have __repr__ and here it was handy too.

lenkaseg commented 4 years ago

Relates to PR #4424 and Issue #4416

pingou commented on line 10 of pagure/api/fork.py 4 years ago

With the .get("repo_from") above the variable repo_from_d may be None which means this line will explode

pingou commented on line 8 of pagure/api/utils.py 4 years ago

Let's remove these two changes (ie: keep the empty lines), they do not relate to this PR :)

pingou commented 4 years ago

I think this is a good start :)

I expect we'll need to adjust some of the tests :)

lenkaseg commented 4 years ago

Let's remove these two changes (ie: keep the empty lines), they do not relate to this PR :)

Ah, yes, that's a residue after removing a ton of prints :)

rebased onto 54f6fe4b1523e593e3e31828ab2606bf3f4ccf97

4 years ago

lenkaseg commented on line 13 of pagure/api/fork.py 4 years ago

Does this fix it?

pingou commented on line 13 of pagure/api/fork.py 4 years ago

You could just check the value of repo_from_d, no? :)

lenkaseg commented 4 years ago

You could just check the value of repo_from_d, no? :)

Ok, I'll redo it.

2 new commits added

Adjust how are passed the arguments about repo_from when opening a new PR
Code style changes and documentation

4 years ago

pretty please pagure-ci rebuild

4 years ago

pretty please pagure-ci rebuild

4 years ago

pretty please pagure-ci rebuild

4 years ago

lenkaseg commented 4 years ago

All the tests pass when I run them in my environment, except the test_style, but that one fails on stuff which is not part of this PR.

rebased onto 5f4a7cd5ca662a3f2879602dfab18547ed85b68f

4 years ago

lenkaseg commented 4 years ago

Rebased on master.

pingou commented 4 years ago

This is what I had to change to make the tests pass:

diff --git a/ pagure/api/fork.py b/ pagure/api/fork.py
index 7ef0a408..9dc6a872 100644
--- a/ pagure/api/fork.py       
+++ b/ pagure/api/fork.py       
@@ -1369,7 +1369,7 @@ def api_pull_request_create(repo, username=None, namespace=None):
           }
         }

-    """
+    """  # noqa

     repo_to = _get_repo(repo, username, namespace)

diff --git a/ tests/test_pagure_flask_api_fork.py b/ tests/test_pagure_flask_api_fork.py
index 8a601a3a..90c0cfc4 100644
--- a/ tests/test_pagure_flask_api_fork.py      
+++ b/ tests/test_pagure_flask_api_fork.py      
@@ -2815,9 +2815,7 @@ class PagureFlaskApiForktests(tests.Modeltests):
         }

         output = self.app.post(
-            "/api/0/test/pull-request/new",
-            headers=headers,
-            data=data,
+            "/api/0/test/pull-request/new", headers=headers, data=data
         )
         self.assertEqual(output.status_code, 200)

diff --git a/ tests/test_pagure_flask_ui_repo.py b/ tests/test_pagure_flask_ui_repo.py
index 96965fce..31dcda65 100644
--- a/ tests/test_pagure_flask_ui_repo.py       
+++ b/ tests/test_pagure_flask_ui_repo.py       
@@ -6757,9 +6757,7 @@ class PagureFlaskRepoTestHooktests(tests.Modeltests):
         """ Test the test_hook endpoint when the user is logged in. """
         user = tests.FakeUser(username="pingou")
         with tests.user_set(self.app.application, user):
-            data = {
-                "csrf_token": self.get_csrf()
-            }
+            data = {"csrf_token": self.get_csrf()}
             output = self.app.post("/test/settings/test_hook", data=data)
             self.assertEqual(output.status_code, 302)

3 new commits added