From e18f6cefb531f953f955a59e7e0283412d8fcbf3 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 18 2017 12:46:41 +0000 Subject: [PATCH 1/11] Add custom drop down lists to custom fields This patch adds an option to create a "List" as a custom field. To do this we need to add "key_data" to the issueKey model for storing the list items.Currently this is only used by "lists", so other field types ignore and remove the data if it's set. https://pagure.io/pagure/issue/1748 --- diff --git a/alembic/versions/38581a8fbae2_add_custom_field_data.py b/alembic/versions/38581a8fbae2_add_custom_field_data.py new file mode 100644 index 0000000..238b487 --- /dev/null +++ b/alembic/versions/38581a8fbae2_add_custom_field_data.py @@ -0,0 +1,22 @@ +"""Add custom field data + +Revision ID: 38581a8fbae2 +Revises: 208b0cd232ab +Create Date: 2017-01-16 13:03:36.683188 + +""" + +# revision identifiers, used by Alembic. +revision = '38581a8fbae2' +down_revision = '208b0cd232ab' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('issue_keys', sa.Column('key_data', sa.Text())) + + +def downgrade(): + op.drop_column('issue_keys', 'key_data') diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index d28b2af..358142d 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3460,9 +3460,9 @@ def save_report(session, repo, name, url, username): session.add(repo) -def set_custom_key_fields(session, project, fields, types): +def set_custom_key_fields(session, project, fields, types, data): """ Set or update the custom key fields of a project with the values - provided. + provided. "data" is currently only used for lists """ current_keys = {} @@ -3470,14 +3470,19 @@ def set_custom_key_fields(session, project, fields, types): current_keys[key.name] = key for idx, key in enumerate(fields): + if types[idx] != "list": + # Only Lists use data, strip it otherwise + data[idx] = "" if key in current_keys: issuekey = current_keys[key] issuekey.key_type = types[idx] + issuekey.key_data = data[idx] else: issuekey = model.IssueKeys( project_id=project.id, name=key, key_type=types[idx], + key_data=data[idx] ) session.add(issuekey) diff --git a/pagure/lib/model.py b/pagure/lib/model.py index f982f9e..79516d7 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -820,6 +820,7 @@ class Issue(BASE): name=field.key.name, key_type=field.key.key_type, value=field.value, + key_data=field.key.key_data ) for field in self.other_fields ] @@ -988,6 +989,7 @@ class IssueKeys(BASE): nullable=False) name = sa.Column(sa.Text(), nullable=False) key_type = sa.Column(sa.String(255), nullable=False) + key_data = sa.Column(sa.Text()) __table_args__ = (sa.UniqueConstraint('project_id', 'name'),) diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html index a2ba336..7864e24 100644 --- a/pagure/templates/issue.html +++ b/pagure/templates/issue.html @@ -367,7 +367,7 @@

- {% if field.name in knowns_keys %} + {% if field.name in knowns_keys %} {% if field.key_type == 'link' %} {% for link in knowns_keys[field.name].value.split(',') %} {{ link }} @@ -383,15 +383,25 @@ {% if authenticated and g.repo_admin %}

- + {% if field.key_type == 'list' %} + + {% else %} + + {% endif %}
{% endif %} {% endfor %} diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index 5955328..90933c2 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -684,7 +684,9 @@

- Set some custom fields for your issues. + Set some custom fields for your issues. Field Values are currently + only used for Lists, and it accepts a comma separated list of items + for the drop down list.

-
+
Fields
+
+ Field Type +
+
+ Field Values (Lists only) +
{% for field in repo.issue_keys or [dict(key_type="", name="")] | sort %}
-
+
-
- + endif %}>Text + endif %}>Boolean + endif %}>Link +
+
+ +
{% endfor %}
@@ -837,7 +852,7 @@

Quick replies will be offered in a new comment form on Issue or - Pull Request page. This allows you to reply to common probles with a + Pull Request page. This allows you to reply to common problems with a click of a button.

The reply can use the same Markdown formatting as regular comments. The list you will choose the reply from will only show the diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 6997e05..aca1db1 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -2289,10 +2289,13 @@ def update_custom_keys(repo, username=None, namespace=None): w.strip() for w in flask.request.form.getlist('custom_keys_type') if w.strip() ] + custom_keys_data = [ + w.strip() for w in flask.request.form.getlist('custom_keys_data') + ] try: msg = pagure.lib.set_custom_key_fields( - SESSION, repo, custom_keys, custom_keys_type) + SESSION, repo, custom_keys, custom_keys_type, custom_keys_data) SESSION.commit() flask.flash(msg) except SQLAlchemyError as err: # pragma: no cover diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index 8ab292e..f33c8ea 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -1900,10 +1900,22 @@ class PagureFlaskApiIssuetests(tests.Modeltests): repo = pagure.lib.get_project(self.session, 'test') msg = pagure.lib.set_custom_key_fields( self.session, repo, - ['bugzilla', 'upstream'], ['link', 'boolean']) + ['bugzilla', 'upstream', 'reviewstatus'], + ['link', 'boolean', 'list'], + ['unused data for non-list type', '', 'ack, nack, needs review']) self.session.commit() self.assertEqual(msg, 'List of custom fields updated') + # Check the project custom fields were correctly set + for key in repo.issue_keys: + # Check that the bugzilla field correctly had its data removed + if key.name == "bugzilla" and key.key_data != "": + assert False + # Check that the reviewstatus list field still has its list + if (key.name == "reviewstatus" and + key.key_data != 'ack, nack, needs review'): + assert False + # No value specified while we try to create the field output = self.app.post( '/api/0/test/issue/1/custom/bugzilla', headers=headers) From 4f181ef2b7621f79b78e729715ad6fa9abedbf3c Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 18 2017 12:46:41 +0000 Subject: [PATCH 2/11] Added docstring to migration script, and used None for the data pruging of non-lists --- diff --git a/alembic/versions/38581a8fbae2_add_custom_field_data.py b/alembic/versions/38581a8fbae2_add_custom_field_data.py index 238b487..925009c 100644 --- a/alembic/versions/38581a8fbae2_add_custom_field_data.py +++ b/alembic/versions/38581a8fbae2_add_custom_field_data.py @@ -15,8 +15,11 @@ import sqlalchemy as sa def upgrade(): + ''' Add a new drop-down list type to the custom fields. This requires us + to store the list items in the issue_keys table. ''' op.add_column('issue_keys', sa.Column('key_data', sa.Text())) def downgrade(): + ''' Remove the key_data column ''' op.drop_column('issue_keys', 'key_data') diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 358142d..4f90167 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3472,7 +3472,7 @@ def set_custom_key_fields(session, project, fields, types, data): for idx, key in enumerate(fields): if types[idx] != "list": # Only Lists use data, strip it otherwise - data[idx] = "" + data[idx] = None if key in current_keys: issuekey = current_keys[key] issuekey.key_type = types[idx] diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index 90933c2..7551b7e 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -733,7 +733,7 @@

+ value={% if field.key_data is none %}""{% else %}"{{ field.key_data }}"{% endif %} class="form-control"/>
{% endfor %} From b47c0362bb8417269aec67293038390fe5e889cc Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 18 2017 12:46:41 +0000 Subject: [PATCH 3/11] Convert field data to json --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 4f90167..67cbf24 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3473,16 +3473,17 @@ def set_custom_key_fields(session, project, fields, types, data): if types[idx] != "list": # Only Lists use data, strip it otherwise data[idx] = None + if key in current_keys: issuekey = current_keys[key] issuekey.key_type = types[idx] - issuekey.key_data = data[idx] + issuekey.data = data[idx] else: issuekey = model.IssueKeys( project_id=project.id, name=key, key_type=types[idx], - key_data=data[idx] + data=data[idx] ) session.add(issuekey) diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 79516d7..d3dde2d 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -999,6 +999,48 @@ class IssueKeys(BASE): 'issue_keys', cascade="delete, delete-orphan", single_parent=True) ) + @property + def data(self): + ''' Return the list of items ''' + items = {} + if self.key_data: + items = json.loads(self.key_data) + else: + return None + return items['list'] + + @property + def data_string(self): + ''' Take the list from key_data and convert it to a string. + This is used by the project settings custom field section. ''' + + if self.key_data: + mylist = json.loads(self.key_data) + else: + return "" + + return_str = "" + first_item = True + if mylist: + for item in mylist['list']: + if first_item: + return_str = item + first_item = False + else: + return_str = return_str + ", " + item + return return_str + + @data.setter + def data(self, list_str): + ''' Ensures the list items are properly saved. ''' + if list_str is None: + self.key_data = None + else: + list_list = [item.strip() for item in list_str.split(',')] + list_data = {} + list_data['list'] = list_list + self.key_data = json.dumps(list_data) + class IssueValues(BASE): """ Stores the values of the custom keys set by project on issues. diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html index 7864e24..bcb3ccd 100644 --- a/pagure/templates/issue.html +++ b/pagure/templates/issue.html @@ -386,7 +386,7 @@ {% if field.key_type == 'list' %} -
+
-
+
+ value={% if field.data is none %}""{% else %}"{{ field.data_string }}"{% endif %} class="form-control"/>
{% endfor %} diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index f33c8ea..6ef7579 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -1902,18 +1902,18 @@ class PagureFlaskApiIssuetests(tests.Modeltests): self.session, repo, ['bugzilla', 'upstream', 'reviewstatus'], ['link', 'boolean', 'list'], - ['unused data for non-list type', '', 'ack, nack, needs review']) + ['unused data for non-list type', '', 'ack, nack , needs review']) self.session.commit() self.assertEqual(msg, 'List of custom fields updated') # Check the project custom fields were correctly set for key in repo.issue_keys: # Check that the bugzilla field correctly had its data removed - if key.name == "bugzilla" and key.key_data != "": + if key.name == "bugzilla" and key.data is not None: assert False # Check that the reviewstatus list field still has its list if (key.name == "reviewstatus" and - key.key_data != 'ack, nack, needs review'): + key.data_string != 'ack, nack, needs review'): assert False # No value specified while we try to create the field From 75e6d9a2e3b5203f4dacd23f5695df36fa69a751 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 18 2017 12:46:41 +0000 Subject: [PATCH 4/11] Store list items in JSON, and move normalization code --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 67cbf24..38962c5 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3473,6 +3473,8 @@ def set_custom_key_fields(session, project, fields, types, data): if types[idx] != "list": # Only Lists use data, strip it otherwise data[idx] = None + else: + data[idx] = [item.strip() for item in data[idx].split(',')] if key in current_keys: issuekey = current_keys[key] diff --git a/pagure/lib/model.py b/pagure/lib/model.py index d3dde2d..19707a4 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1002,44 +1002,18 @@ class IssueKeys(BASE): @property def data(self): ''' Return the list of items ''' - items = {} if self.key_data: - items = json.loads(self.key_data) + return json.loads(self.key_data) else: return None - return items['list'] - - @property - def data_string(self): - ''' Take the list from key_data and convert it to a string. - This is used by the project settings custom field section. ''' - - if self.key_data: - mylist = json.loads(self.key_data) - else: - return "" - - return_str = "" - first_item = True - if mylist: - for item in mylist['list']: - if first_item: - return_str = item - first_item = False - else: - return_str = return_str + ", " + item - return return_str @data.setter def data(self, list_str): - ''' Ensures the list items are properly saved. ''' + ''' Store the list in JSON. ''' if list_str is None: self.key_data = None else: - list_list = [item.strip() for item in list_str.split(',')] - list_data = {} - list_data['list'] = list_list - self.key_data = json.dumps(list_data) + self.key_data = json.dumps(list_str) class IssueValues(BASE): diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index 740c21c..a0a90c3 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -733,7 +733,7 @@
+ value={% if field.data is none %}""{% else %}"{{ field.data | join(', ') }}"{% endif %} class="form-control"/>
{% endfor %} diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index 6ef7579..1a75cb7 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -1911,10 +1911,12 @@ class PagureFlaskApiIssuetests(tests.Modeltests): # Check that the bugzilla field correctly had its data removed if key.name == "bugzilla" and key.data is not None: assert False + # Check that the reviewstatus list field still has its list - if (key.name == "reviewstatus" and - key.data_string != 'ack, nack, needs review'): - assert False + if (key.name == "reviewstatus"): + for item in ['ack', 'nack', 'needs review']: + if item not in key.data: + assert False # No value specified while we try to create the field output = self.app.post( From 6fac0c0013247a4a704c5803c3ffd22580030925 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 18 2017 12:46:41 +0000 Subject: [PATCH 5/11] Adjust test case --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 19707a4..41d7493 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1008,12 +1008,12 @@ class IssueKeys(BASE): return None @data.setter - def data(self, list_str): - ''' Store the list in JSON. ''' - if list_str is None: + def data(self, data_obj): + ''' Store the list data in JSON. ''' + if data_obj is None: self.key_data = None else: - self.key_data = json.dumps(list_str) + self.key_data = json.dumps(data_obj) class IssueValues(BASE): diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index 1a75cb7..3b71ea3 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -1909,14 +1909,13 @@ class PagureFlaskApiIssuetests(tests.Modeltests): # Check the project custom fields were correctly set for key in repo.issue_keys: # Check that the bugzilla field correctly had its data removed - if key.name == "bugzilla" and key.data is not None: - assert False + if key.name == "bugzilla": + self.assertTrue(key.data is None) # Check that the reviewstatus list field still has its list if (key.name == "reviewstatus"): for item in ['ack', 'nack', 'needs review']: - if item not in key.data: - assert False + self.assertFalse(item not in key.data) # No value specified while we try to create the field output = self.app.post( From cb289d938f5c28301c1df087334c82b40d6d0d56 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 18 2017 12:46:41 +0000 Subject: [PATCH 6/11] Revise comment in migration script --- diff --git a/alembic/versions/38581a8fbae2_add_custom_field_data.py b/alembic/versions/38581a8fbae2_add_custom_field_data.py index 925009c..1090b04 100644 --- a/alembic/versions/38581a8fbae2_add_custom_field_data.py +++ b/alembic/versions/38581a8fbae2_add_custom_field_data.py @@ -15,8 +15,8 @@ import sqlalchemy as sa def upgrade(): - ''' Add a new drop-down list type to the custom fields. This requires us - to store the list items in the issue_keys table. ''' + ''' Add a new drop-down list type to the custom fields. The list options + need to be stored in the issue_keys table. ''' op.add_column('issue_keys', sa.Column('key_data', sa.Text())) From 411167f5c97c476665e58f51e437cdaa40f753b1 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 18 2017 12:46:41 +0000 Subject: [PATCH 7/11] Fix testcase --- diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index 3b71ea3..fb3e277 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -1913,9 +1913,9 @@ class PagureFlaskApiIssuetests(tests.Modeltests): self.assertTrue(key.data is None) # Check that the reviewstatus list field still has its list - if (key.name == "reviewstatus"): + if key.name == "reviewstatus": for item in ['ack', 'nack', 'needs review']: - self.assertFalse(item not in key.data) + self.assertTrue(item in key.data) # No value specified while we try to create the field output = self.app.post( From ff0ba200ae6f7f738b43355181f75d697fbb018a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 18 2017 12:46:41 +0000 Subject: [PATCH 8/11] Small code style change in the unit-tests --- diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index fb3e277..5ff71b6 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -1914,8 +1914,8 @@ class PagureFlaskApiIssuetests(tests.Modeltests): # Check that the reviewstatus list field still has its list if key.name == "reviewstatus": - for item in ['ack', 'nack', 'needs review']: - self.assertTrue(item in key.data) + self.assertEqual( + sorted(key.data), ['ack', 'nack', 'needs review']) # No value specified while we try to create the field output = self.app.post( From aafbc2a036475e92d2623db0b85f4d66de7ba6e7 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 18 2017 12:47:45 +0000 Subject: [PATCH 9/11] Fix the alembic migration script to keep the history linear --- diff --git a/alembic/versions/38581a8fbae2_add_custom_field_data.py b/alembic/versions/38581a8fbae2_add_custom_field_data.py index 1090b04..63ca6b3 100644 --- a/alembic/versions/38581a8fbae2_add_custom_field_data.py +++ b/alembic/versions/38581a8fbae2_add_custom_field_data.py @@ -1,14 +1,14 @@ """Add custom field data Revision ID: 38581a8fbae2 -Revises: 208b0cd232ab +Revises: 6addaed6008e Create Date: 2017-01-16 13:03:36.683188 """ # revision identifiers, used by Alembic. revision = '38581a8fbae2' -down_revision = '208b0cd232ab' +down_revision = '6addaed6008e' from alembic import op import sqlalchemy as sa From cb755e1e1afefcb86c9101395f2eac172ed0a199 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 18 2017 12:53:42 +0000 Subject: [PATCH 10/11] Fix the unit-tests --- diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index ff6e001..bb1d0d7 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -1651,11 +1651,13 @@ index 0000000..60f7480 "name": "custom1", "key_type": "text", "value": "value1", + "key_data": None, }, { "name": "custom2", "key_type": "text", "value": "value2", + "key_data": None, } ] From 57c0da4f62c868084f04775cc7e492d14d5c0b0d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 18 2017 13:16:37 +0000 Subject: [PATCH 11/11] Move the custom field block to the right and fix indentation in the html --- diff --git a/pagure/static/pagure.css b/pagure/static/pagure.css index 695e345..255c040 100644 --- a/pagure/static/pagure.css +++ b/pagure/static/pagure.css @@ -3,6 +3,10 @@ html font-size:14px; } +.right { + float: right; +} + .disabled a { pointer-events: none; color: #808080; diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html index bcb3ccd..b5ab9dc 100644 --- a/pagure/templates/issue.html +++ b/pagure/templates/issue.html @@ -194,12 +194,12 @@ assignee=issue.assignee.username) }}"> {{ issue.assignee.username }} - {% if authenticated and (issue.assignee.username == g.fas_user.username) %} - - {% endif %} + {% if authenticated and (issue.assignee.username == g.fas_user.username) %} + + {% endif %} {% else %} unassigned {% endif %} @@ -246,7 +246,6 @@ {% endif%} -
@@ -358,7 +357,7 @@ {% endif %} {% if repo.issue_keys %} -
+