Update form for adding distribution #639

Zlopez · 2018-10-17T10:28:30Z

This PR implements #594.

What was changed:

To add distribution through distros page user no longer needs to be admin
Distribution in mapping page is now selected from dropdown, not automatically created if it doesn't exists
Add confirmation dialog to /distro/add page if there are distributions with similar names (this is also showed on the page itself before submit)
Polished /distro/add and /project/<id>/map pages to look a little better
Add information where user can add new distribution directly to /project/<id>/map
Fix failing test

This changes should prevent making distributions containing typos and duplicates of already existing distributions.

codecov-io · 2018-10-17T10:31:28Z

Codecov Report

Merging #639 into master will decrease coverage by <.01%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master     #639      +/-   ##
==========================================
- Coverage   90.55%   90.54%   -0.01%     
==========================================
  Files          56       56              
  Lines        2690     2698       +8     
  Branches      352      355       +3     
==========================================
+ Hits         2436     2443       +7     
  Misses        197      197              
- Partials       57       58       +1

Impacted Files	Coverage Δ
anitya/admin.py	`82.87% <ø> (-1.39%)`	⬇️
anitya/ui.py	`79.37% <100%> (+1.74%)`	⬆️
anitya/forms.py	`79.06% <66.66%> (-1.42%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c58c68...95f7337. Read the comment docs.

bowlofeggs · 2018-10-19T20:05:22Z

news/PR639.feature

@@ -0,0 +1 @@
+Update form for adding new distribution


Suggested change

Update form for adding new distribution

Update form for adding new distributions

bowlofeggs · 2018-10-19T20:06:20Z

anitya/forms.py

+    distro = SelectField(
+        'Distribution',
+        [validators.DataRequired()],
+        choices=[(item, item) for item in []])


What express it this way? Isn't this basically just saying choices=[]?

bowlofeggs · 2018-10-19T20:09:03Z

anitya/tests/test_flask.py

+        """Assert admins can add distributions."""
+        with login_user(self.flask_app, self.user):
+            output = self.client.get('/distro/add')
+            print(output.data)


Suggested change

print(output.data)

bowlofeggs · 2018-10-19T20:09:20Z

anitya/tests/test_flask.py

+
+            output = self.client.post('/distro/add', data=data, follow_redirects=True)
+
+            # self.assertEqual(201, output.status_code)


Suggested change

# self.assertEqual(201, output.status_code)

bowlofeggs · 2018-10-19T20:10:00Z

anitya/tests/test_flask.py

+                b'name="csrf_token" type="hidden" value="')[1].split(b'">')[0]
+            data = {'name': 'Fedora', 'csrf_token': csrf_token}
+
+            output = self.client.post('/distro/add', data=data, follow_redirects=True)


Does self.client.post() assert that the return code is 200 by default?

bowlofeggs · 2018-10-19T20:11:39Z

anitya/tests/test_flask.py

+            dup_output = self.client.post('/distro/add', data=data, follow_redirects=True)
+
+            self.assertTrue(b'Distribution added' in create_output.data)
+            self.assertTrue(b'Could not add this distro' in dup_output.data)


It seems like it'd be good to assert that an HTTP error code was used here. Perhaps 409 (conflict)?

Suggested change

self.assertTrue(b'Could not add this distro' in dup_output.data)

self.assertTrue(b'Could not add this distro' in dup_output.data)

self.assertEqual(409, output.status_code)

The user is redirected to /distros page, so the user gets 301 redirect.

bowlofeggs

It would also be helpful to give more information in the commit message, and to make the title more descriptive. What are we updating it to do that it doesn't do now?

bowlofeggs

LGTM, though I recommend squashing the commits before merging.

Zlopez · 2018-10-24T08:13:25Z

squash

bowlofeggs

This one also fails in Travis - does the master branch fail linting?

* To add distribution through distros page user no longer needs to be admin * Distribution in mapping page is now selected from dropdown, not automatically created if it doesn't exists * Add confirmation dialog to `/distro/add` page if there are distributions with similar names (this is also showed on the page itself before submit) * Polished `/distro/add` and `/project/<id>/map` pages to look a little better * Add information where user can add new distribution directly to `/project/<id>/map` * Fix failing test

Zlopez · 2018-10-25T13:55:25Z

The PR fixing the lint issue was merged, so I'm doing a rebase to fix also this PR.

Zlopez force-pushed the distro_form branch from bd7790e to 5eaf434 Compare October 17, 2018 10:31

bowlofeggs suggested changes Oct 19, 2018

View reviewed changes

Zlopez force-pushed the distro_form branch from 5eaf434 to 7fb98a3 Compare October 22, 2018 12:38

bowlofeggs reviewed Oct 23, 2018

View reviewed changes

Zlopez force-pushed the distro_form branch from 7fb98a3 to 3b06dd9 Compare October 24, 2018 08:13

bowlofeggs previously approved these changes Oct 24, 2018

View reviewed changes

Zlopez dismissed bowlofeggs’s stale review via f617042 October 25, 2018 13:55

Zlopez force-pushed the distro_form branch from 3b06dd9 to f617042 Compare October 25, 2018 13:55

bowlofeggs approved these changes Oct 26, 2018

View reviewed changes

Merge branch 'master' into 'distro_form'

95f7337

mergify bot merged commit 0994242 into fedora-infra:master Oct 26, 2018

Zlopez deleted the distro_form branch October 29, 2018 15:36

Zlopez mentioned this pull request Nov 1, 2018

Change the way how distribution is added #622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update form for adding distribution #639

Update form for adding distribution #639

Zlopez commented Oct 17, 2018

codecov-io commented Oct 17, 2018 •

edited

Loading

bowlofeggs Oct 19, 2018

bowlofeggs Oct 19, 2018

bowlofeggs Oct 19, 2018

bowlofeggs Oct 19, 2018

bowlofeggs Oct 19, 2018

bowlofeggs Oct 19, 2018

Zlopez Oct 22, 2018

bowlofeggs left a comment

bowlofeggs left a comment

Zlopez commented Oct 24, 2018

bowlofeggs left a comment

Zlopez commented Oct 25, 2018

	Update form for adding new distribution
	Update form for adding new distributions


		output = self.client.post('/distro/add', data=data, follow_redirects=True)

		# self.assertEqual(201, output.status_code)

	self.assertTrue(b'Could not add this distro' in dup_output.data)
	self.assertTrue(b'Could not add this distro' in dup_output.data)
	self.assertEqual(409, output.status_code)

Update form for adding distribution #639

Update form for adding distribution #639

Conversation

Zlopez commented Oct 17, 2018

codecov-io commented Oct 17, 2018 • edited Loading

Codecov Report

bowlofeggs Oct 19, 2018

Choose a reason for hiding this comment

bowlofeggs Oct 19, 2018

Choose a reason for hiding this comment

bowlofeggs Oct 19, 2018

Choose a reason for hiding this comment

bowlofeggs Oct 19, 2018

Choose a reason for hiding this comment

bowlofeggs Oct 19, 2018

Choose a reason for hiding this comment

bowlofeggs Oct 19, 2018

Choose a reason for hiding this comment

Zlopez Oct 22, 2018

Choose a reason for hiding this comment

bowlofeggs left a comment

Choose a reason for hiding this comment

bowlofeggs left a comment

Choose a reason for hiding this comment

Zlopez commented Oct 24, 2018

bowlofeggs left a comment

Choose a reason for hiding this comment

Zlopez commented Oct 25, 2018

codecov-io commented Oct 17, 2018 •

edited

Loading