#2378 Redirect /issue/ and /api/issue to /issues/ and /api/issues
Closed 5 years ago by vivekanand1101. Opened 6 years ago by vivekanand1101.
vivekanand1101/pagure redirect  into  master

file modified
+18
@@ -8,6 +8,12 @@ 

  

  """

  

+ 

+ # pylint: disable=too-many-lines

+ # pylint: disable=too-many-branches

+ # pylint: disable=too-many-locals

+ # pylint: disable=too-many-statements

+ 

  import datetime

  import flask

  
@@ -263,6 +269,18 @@ 

      return jsonout

  

  

+ @API.route('/<namespace>/<repo>/issue')

+ @API.route('/fork/<username>/<repo>/issue')

+ @API.route('/<repo>/issue')

+ @API.route('/fork/<username>/<namespace>/<repo>/issue')

+ @api_login_optional()

+ @api_method

+ def api_view_issues_redirect(repo, username=None, namespace=None):

+     """ List the issues in the project """

+     return flask.redirect(flask.url_for(

+         'api_ns.api_view_issues', repo=repo, username=username, namespace=namespace))

+ 

+ 

  @API.route('/<namespace>/<repo>/issues')

  @API.route('/fork/<username>/<repo>/issues')

  @API.route('/<repo>/issues')

file modified
+15
@@ -589,6 +589,21 @@ 

      )

  

  

+ @APP.route('/<repo>/issue/')

+ @APP.route('/<repo>/issue')

+ @APP.route('/<namespace>/<repo>/issue/')

+ @APP.route('/<namespace>/<repo>/issue')

+ @APP.route('/fork/<username>/<repo>/issue/')

+ @APP.route('/fork/<username>/<repo>/issue')

+ @APP.route('/fork/<username>/<namespace>/<repo>/issue/')

+ @APP.route('/fork/<username>/<namespace>/<repo>/issue')

+ def view_issues_redirect(repo, username=None, namespace=None):

+     """ List all issues associated to a repo

+     """

+     return flask.redirect(flask.url_for(

+         'view_issues', repo=repo, username=username, namespace=namespace))

+ 

+ 

  @APP.route('/<repo>/issues/')

  @APP.route('/<repo>/issues')

  @APP.route('/<namespace>/<repo>/issues/')

@@ -797,6 +797,52 @@ 

              }

          )

  

+     def test_api_view_issues_redirect(self):

+         """ Test the api_view_issues redirect method of the flask api. """

+         self.test_api_new_issue()

I think it is better and nicer to override the setUp() method of the class instead of calling a test.
you have an example here https://pagure.io/pagure/blob/master/f/tests/test_pagure_lib.py#_152

+ 

+         output = self.app.get('/api/0/foo/issue')

+         self.assertEqual(output.status_code, 302)

+         # Invalid repo

+         output = self.app.get('/api/0/foo/issue', follow_redirects=True)

+         self.assertEqual(output.status_code, 404)

+         data = json.loads(output.data)

+         self.assertDictEqual(

+             data,

+             {

+               "error": "Project not found",

+               "error_code": "ENOPROJECT",

+             }

+         )

+ 

+         # List all opened issues

+         output = self.app.get('/api/0/test/issue')

+         self.assertEqual(output.status_code, 302)

+ 

+         output = self.app.get('/api/0/test/issue', follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         data = json.loads(output.data)

+         for idx in range(len(data['issues'])):

+             data['issues'][idx]['date_created'] = '1431414800'

+             data['issues'][idx]['last_updated'] = '1431414800'

+         self.assertDictEqual(

+             data,

+             {

+               "args": {

+                 "assignee": None,

+                 "author": None,

+                 'milestones': [],

+                 'no_stones': None,

+                 'priority': None,

+                 "since": None,

+                 "status": None,

+                 "tags": [],

+               },

+               "issues": FULL_ISSUE_LIST[3:],

+               "total_issues": 6

+             }

+         )

+ 

      def test_api_view_issues(self):

          """ Test the api_view_issues method of the flask api. """

          self.test_api_new_issue()

@@ -423,6 +423,34 @@ 

  

      @patch('pagure.lib.git.update_git')

      @patch('pagure.lib.notify.send_email')

+     def test_view_issues_redirect(self, p_send_email, p_ugt):

+         """ Test the view_issues endpoint. """

+         p_send_email.return_value = True

+         p_ugt.return_value = True

+ 

+         output = self.app.get('/foo/issue')

+         self.assertEqual(output.status_code, 302)

+ 

+         output = self.app.get('/foo/issue', follow_redirects=True)

+         self.assertEqual(output.status_code, 404)

+ 

+         tests.create_projects(self.session)

+         tests.create_projects_git(

+             os.path.join(self.path, 'repos'), bare=True)

+ 

+         output = self.app.get('/test/issue')

+         self.assertEqual(output.status_code, 302)

+ 

+         output = self.app.get('/test/issue', follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn(

+             'div class="projectinfo m-t-1 m-b-1">\ntest project #1        '

+             '</div>', output.data)

+         self.assertTrue(

+             '<h2>\n      0 Open Issues' in output.data)

+ 

+     @patch('pagure.lib.git.update_git')

+     @patch('pagure.lib.notify.send_email')

      def test_view_issues(self, p_send_email, p_ugt):

          """ Test the view_issues endpoint. """

          p_send_email.return_value = True

no initial comment

Some problems with tests are there.

If we go this way, we could probably use 301 redirects instead of 302

I think redirects in APIs are a bad idea and likely to cause confusion.

I do think this one's a good idea. Maybe also add it for /pull-request/ ?

So you would favor the status-quo or the discrepancy between UI and API ?

Correct. I think that's reasonable, due to the differences in clients using them.
The API tends to be automated clients, often with preconfigured URLs, whereas users sometimes try to enter the url themselves, and I think we can help them along to find what they want there.

I think it is better and nicer to override the setUp() method of the class instead of calling a test.
you have an example here https://pagure.io/pagure/blob/master/f/tests/test_pagure_lib.py#_152

rebased

6 years ago

Correct. I think that's reasonable, due to the differences in clients using them. The API tends to be automated clients, often with preconfigured URLs, whereas users sometimes try to enter the url themselves, and I think we can help them along to find what they want there.

@puiterwijk but, there shouldn't be any client which are currently using this api endpoint, no?

I am tempted to close this PR, it has not been updated in 10 months and just seats in the PR queue.

What do you think?

rebased onto 121e3db

5 years ago

yeah +1 for closing. we can always reopen if work starts on it again.

Pull-Request has been closed by vivekanand1101

5 years ago