#4462 Let the person who opens a PR allow the maintainers of the target project to rebase a PR
Merged 4 months ago by pingou. Opened 4 months ago by pingou.

@@ -0,0 +1,34 @@ 

+ """Add support for allow_rebase

+ 

+ Revision ID: d7589827abbb

+ Revises: 802047d28f89

+ Create Date: 2019-05-09 16:25:58.971712

+ 

+ """

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ 

+ 

+ # revision identifiers, used by Alembic.

+ revision = 'd7589827abbb'

+ down_revision = '802047d28f89'

+ 

+ 

+ def upgrade():

+     ''' Add the column allow_rebase to the table pull_requests.

+     '''

+     op.add_column(

+         'pull_requests',

+         sa.Column('allow_rebase', sa.Boolean, default=False, nullable=True)

+     )

+     op.execute('''UPDATE "pull_requests" SET allow_rebase=False;''')

+     op.alter_column(

+         'pull_requests', 'allow_rebase',

+         nullable=False, existing_nullable=True)

+ 

+ 

+ def downgrade():

+     ''' Remove the column allow_rebase from the table pull_requests.

+     '''

+     op.drop_column('pull_requests', 'allow_rebase')

file modified
+2 -1

@@ -81,7 +81,7 @@ 

      EISSUENOTALLOWED = "You are not allowed to view this issue"

      EPRNOTALLOWED = "You are not allowed to view this pull-request"

      EPULLREQUESTSDISABLED = (

-         "Pull-Request have been deactivated for this " "project"

+         "Pull-Request have been deactivated for this project"

      )

      ENOREQ = "Pull-Request not found"

      ENOPRCLOSE = (

@@ -122,6 +122,7 @@ 

      ETRACKERREADONLY = "The issue tracker of this project is read-only"

      ENOPRSTATS = "No statistics could be computed for this PR"

      EUBLOCKED = "You have been blocked from this project"

+     EREBASENOTALLOWED = "You are not authorized to rebase this pull-request"

  

  

  def get_authorized_api_project(session, repo, user=None, namespace=None):

file modified
+6 -1

@@ -665,11 +665,16 @@ 

      repo = _get_repo(repo, username, namespace)

      _check_pull_request(repo)

      _check_token(repo)

-     _get_request(repo, requestid)

+     request = _get_request(repo, requestid)

  

      if not is_repo_committer(repo):

          raise pagure.exceptions.APIError(403, error_code=APIERROR.ENOPRCLOSE)

  

+     if not request.allow_rebase:

+         raise pagure.exceptions.APIError(

+             403, error_code=APIERROR.EREBASENOTALLOWED

+         )

+ 

      task = pagure.lib.tasks.rebase_pull_request.delay(

          repo.name,

          namespace,

file modified
+5

@@ -315,6 +315,11 @@ 

      initial_comment = wtforms.TextAreaField(

          "Initial Comment", [wtforms.validators.Optional()]

      )

+     allow_rebase = wtforms.BooleanField(

+         "Allow rebasing",

+         [wtforms.validators.Optional()],

+         false_values=FALSE_VALUES,

+     )

  

  

  class RemoteRequestPullForm(RequestPullForm):

file modified
+17 -7

@@ -1964,13 +1964,23 @@ 

          # Push the changes

          _log.info("Pushing %s to %s", branch_ref.name, request.branch_from)

          try:

-             tempclone.push(

-                 username,

-                 branch_ref.name,

-                 request.branch_from,

-                 pull_request=request,

-                 force=True,

-             )

+             if request.allow_rebase:

+                 tempclone.push(

+                     username,

+                     branch_ref.name,

+                     request.branch_from,

+                     pull_request=request,

+                     force=True,

+                     internal="yes",

+                 )

+             else:

+                 tempclone.push(

+                     username,

+                     branch_ref.name,

+                     request.branch_from,

+                     pull_request=request,

+                     force=True,

+                 )

          except subprocess.CalledProcessError as err:

              _log.debug(

                  "Rebase FAILED: {cmd} returned code {code} with the "

file modified
+2

@@ -1960,6 +1960,8 @@ 

          nullable=True,

      )

  

+     allow_rebase = sa.Column(sa.Boolean, default=False, nullable=False)

+ 

      # While present this column isn't used anywhere yet

      private = sa.Column(sa.Boolean, nullable=False, default=False)

  

file modified
+2

@@ -1859,6 +1859,7 @@ 

      title,

      user,

      initial_comment=None,

+     allow_rebase=False,

      repo_from=None,

      remote_git=None,

      requestuid=None,

@@ -1887,6 +1888,7 @@ 

          branch_from=branch_from,

          title=title,

          initial_comment=initial_comment or None,

+         allow_rebase=allow_rebase,

          user_id=user_obj.id,

          status=status,

          commit_start=commit_start,

@@ -193,10 +193,10 @@ 

            </div>

          </div>

          <div class="card-body">

-             <textarea class="form-control" rows=8 id="initial_comment" name="initial_comment"

+           <textarea class="form-control" rows=8 id="initial_comment" name="initial_comment"

              placeholder="Describe your changes" tabindex=1>

              {{- form.initial_comment.data if form.initial_comment.data else '' -}}

-             </textarea>

+           </textarea>

            {% if form.initial_comment.errors %}

            <span class="float-right text-danger">

              <small>

@@ -206,24 +206,33 @@ 

              </small>

            </span>

            {% endif %}

-             <div id="preview" class="p-1">

-             </div>

+           <div id="preview" class="p-1">

+           </div>

+           <div class="form-control">

+             <label for="allow_rebase">Allow rebasing</label>

+             <label class="c-input c-checkbox">

+               <input checked id="allow_rebase" name="allow_rebase" type="checkbox" value="y">

+             </label>

+             <small class="text-muted">

+               Let the maintainer of the target project to rebase the pull-request

+             </small>

            </div>

-           <div class="card-footer bg-light">

-             <div class="d-flex align-items-center">

-               <small>Comments use <a href="https://docs.pagure.org/pagure/usage/markdown.html"

-                   target="_blank" rel="noopener noreferrer" class="notblue">Markdown Syntax</a></small>

-               <div class="ml-auto">

-                 <div class="btn-group">

-                     <input type="submit" class="btn btn-primary" value="Create Pull Request"{%

-                       if not diff %} disabled title="There appear to be no diff, so nothing to request pulling"{% endif %}>

+         </div>

+         <div class="card-footer bg-light">

+           <div class="d-flex align-items-center">

+             <small>Comments use <a href="https://docs.pagure.org/pagure/usage/markdown.html"

+                 target="_blank" rel="noopener noreferrer" class="notblue">Markdown Syntax</a></small>

+             <div class="ml-auto">

+               <div class="btn-group">

+                   <input type="submit" class="btn btn-primary" value="Create Pull Request"{%

+                     if not diff %} disabled title="There appear to be no diff, so nothing to request pulling"{% endif %}>

  

-                 </div>

                </div>

              </div>

            </div>

          </div>

-         {{ form.csrf_token }}

+       </div>

+       {{ form.csrf_token }}

      </div>

  

  

@@ -179,7 +179,7 @@ 

            </a>

            <div id="merge-alert" class="text-xs-center dropdown-menu dropdown-menu-right p-0" style="min-width:400px">

               <div class="alert text-center mb-0">

-                 {% if pull_request.status == 'Open' and g.repo_committer %}

+                 {% if pull_request.status == 'Open' and g.repo_committer and pull_request.allow_rebase %}

                  <small id="merge-alert-message"></small>

                  <form action="{{ url_for('ui_ns.merge_request_pull',

                          repo=repo.name,

file modified
+3

@@ -1712,6 +1712,7 @@ 

                  repo_from=repo,

                  title=form.title.data,

                  initial_comment=initial_comment,

+                 allow_rebase=form.allow_rebase.data,

                  user=flask.g.fas_user.username,

                  commit_start=commit_start,

                  commit_stop=commit_stop,

@@ -1756,6 +1757,8 @@ 

  

      if not flask.g.repo_committer:

          form = None

+     elif flask.request.method == "GET":

+         form.allow_rebase.data = True

  

      # if the pull request we are creating only has one commit,

      # we automatically fill out the form fields for the PR with

@@ -241,7 +241,7 @@ 

          output = self.app.get("/api/0/-/error_codes")

          self.assertEqual(output.status_code, 200)

          data = json.loads(output.get_data(as_text=True))

-         self.assertEqual(len(data), 36)

+         self.assertEqual(len(data), 37)

          self.assertEqual(

              sorted(data.keys()),

              sorted(

@@ -282,6 +282,7 @@ 

                      "ETRACKERDISABLED",

                      "ETRACKERREADONLY",

                      "EUBLOCKED",

+                     "EREBASENOTALLOWED",

                  ]

              ),

          )

@@ -68,6 +68,7 @@ 

              branch_to="master",

              title="PR from the test branch",

              user="pingou",

+             allow_rebase=True,

          )

          self.session.commit()

          self.assertEqual(req.id, 1)

@@ -400,5 +401,153 @@ 

          )

  

  

+ class PagureRebaseNotAllowedtests(tests.Modeltests):

+     """ Tests rebasing pull-request in pagure """

+ 

+     maxDiff = None

+ 

+     @patch("pagure.lib.notify.send_email", MagicMock(return_value=True))

+     def setUp(self):

+         """ Set up the environnment, ran before every tests. """

+         super(PagureRebaseNotAllowedtests, self).setUp()

+ 

+         pagure.config.config["REQUESTS_FOLDER"] = None

+         tests.create_projects(self.session)

+         tests.create_projects_git(os.path.join(self.path, "repos"), bare=True)

+         tests.create_projects_git(

+             os.path.join(self.path, "requests"), bare=True

+         )

+         tests.add_content_to_git(

+             os.path.join(self.path, "repos", "test.git"),

+             branch="master",

+             content="foobarbaz",

+             filename="testfile",

+         )

+         tests.add_content_to_git(

+             os.path.join(self.path, "repos", "test.git"),

+             branch="test",

+             content="foobar",

+             filename="sources",

+         )

+         tests.add_readme_git_repo(os.path.join(self.path, "repos", "test.git"))

+ 

+         # Create a PR for these changes

+         project = pagure.lib.query.get_authorized_project(self.session, "test")

+         req = pagure.lib.query.new_pull_request(

+             session=self.session,

+             repo_from=project,

+             branch_from="test",

+             repo_to=project,

+             branch_to="master",

+             title="PR from the test branch",

+             user="pingou",

+             allow_rebase=False,

+         )

+         self.session.commit()

+         self.assertEqual(req.id, 1)

+         self.assertEqual(req.title, "PR from the test branch")

+ 

+         self.project = pagure.lib.query.get_authorized_project(

+             self.session, "test"

+         )

+         self.assertEqual(len(project.requests), 1)

+         self.request = self.project.requests[0]

+ 

+     def test_rebase_api_ui_logged_in(self):

+         """ Test the rebase PR API endpoint when logged in from the UI and

+         its outcome. """

+ 

+         user = tests.FakeUser(username="pingou")

+         with tests.user_set(self.app.application, user):

+             # Get the merge status first so it's cached and can be refreshed

+             csrf_token = self.get_csrf()

+             data = {"requestid": self.request.uid, "csrf_token": csrf_token}

+             output = self.app.post("/pv/pull-request/merge", data=data)

+             self.assertEqual(output.status_code, 200)

+             data = json.loads(output.get_data(as_text=True))

+             self.assertEqual(

+                 data,

+                 {

+                     "code": "MERGE",

+                     "message": "The pull-request can be merged with "

+                     "a merge commit",

+                     "short_code": "With merge",

+                 },

+             )

+ 

+             output = self.app.post("/api/0/test/pull-request/1/rebase")

+             self.assertEqual(output.status_code, 403)

+             data = json.loads(output.get_data(as_text=True))

+             self.assertEqual(

+                 data,

+                 {

+                     "error": "You are not authorized to rebase this pull-request",

+                     "error_code": "EREBASENOTALLOWED",

+                 },

+             )

+ 

+     def test_rebase_api_ui_logged_in_different_user(self):

+         """ Test the rebase PR API endpoint when logged in from the UI and

+         its outcome. """

+         # Add 'foo' to the project 'test' so 'foo' can rebase the PR

+         repo = pagure.lib.query._get_project(self.session, "test")

+         msg = pagure.lib.query.add_user_to_project(

+             session=self.session, project=repo, new_user="foo", user="pingou"

+         )

+         self.session.commit()

+         self.assertEqual(msg, "User added")

+ 

+         user = tests.FakeUser(username="foo")

+         with tests.user_set(self.app.application, user):

+             # Get the merge status first so it's cached and can be refreshed

+             csrf_token = self.get_csrf()

+             data = {"requestid": self.request.uid, "csrf_token": csrf_token}

+             output = self.app.post("/pv/pull-request/merge", data=data)

+             self.assertEqual(output.status_code, 200)

+             data = json.loads(output.get_data(as_text=True))

+             self.assertEqual(

+                 data,

+                 {

+                     "code": "MERGE",

+                     "message": "The pull-request can be merged with "

+                     "a merge commit",

+                     "short_code": "With merge",

+                 },

+             )

+ 

+             output = self.app.post("/api/0/test/pull-request/1/rebase")

+             self.assertEqual(output.status_code, 403)

+             data = json.loads(output.get_data(as_text=True))

+             self.assertEqual(

+                 data,

+                 {

+                     "error": "You are not authorized to rebase this pull-request",

+                     "error_code": "EREBASENOTALLOWED",

+                 },

+             )

+ 

+     def test_rebase_api_api_logged_in(self):

+         """ Test the rebase PR API endpoint when using an API token and

+         its outcome. """

+ 

+         tests.create_tokens(self.session)

+         tests.create_tokens_acl(self.session)

+ 

+         headers = {"Authorization": "token aaabbbcccddd"}

+ 

+         output = self.app.post(

+             "/api/0/test/pull-request/1/rebase", headers=headers

+         )

+         self.assertEqual(output.status_code, 403)

+         data = json.loads(output.get_data(as_text=True))

+         self.assertEqual(

+             data,

+             {

+                 "error": "You are not authorized to rebase this pull-request",

+                 "error_code": "EREBASENOTALLOWED",

+             },

+         )

+ 

+ 

  if __name__ == "__main__":

      unittest.main(verbosity=2)

@@ -2558,7 +2558,7 @@ 

                  """<textarea class="form-control" rows=8 id="initial_comment" name="initial_comment"

              placeholder="Describe your changes" tabindex=1>

  More information</textarea>

-             <div id="preview" class="p-1">""",

+           <div id="preview" class="p-1">""",

                  output_text,

              )

              self.assertIn(

rebased onto 54a4b89353524f0840e45b1ec24f17de2c6daf91

4 months ago

rebased onto 548578607ffe5565f9c9f419698b50cffd2c34ed

4 months ago

rebased onto 03793dfbdda31b41f0215647ebcce8177a9b27f0

4 months ago

I'm not sure it is relevant but shouldn't there be a test added for this?

rebased onto 3e6dbcd

4 months ago

The fact that the current tests are passing is a basis, I'll see if I can add some extra ones :)

4 new commits added

  • Do not allow rebase via the API if the PR does not allow it
  • Fix description of the API error EPULLREQUESTSDISABLED
  • Add support for allowing the maintainers of the target project rebase
  • Adjust HTML indentations
4 months ago

Pull-Request has been merged by pingou

4 months ago