From 403d8f00743190c998eff89a61b078a3bad03289 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 1/9] Issue 1544 - RFE - Add colored tags I only extended the base Tag object to include color, projectID, and a unique id. Then I removed the dependency of the issue from the tag for working in the ui. Otherwise tags were only visible if an issue contained that tag. This also meant you could not add, edit, or remove a tag unless you had already added one to an issue. So in my patch it updated the setting.html page to show all tags (whether or not they existed in an issue). You can add, edit and delete them. They also automatically update all the issues in that project with the color/name change. This is also reflected in both issues.html & issue.html. I also added a unique identifier to each tag(tag_id) - this allows every project to use the same name with its own unique color. There are no conflicts since the primary key is now the unique identifier(tag_id) instead of the tag name(tag). This patch enforces that tags are only created from the settings page. This patch is missing the complete alembic migration script. --- diff --git a/pagure/default_config.py b/pagure/default_config.py index 2aed1c3..7fb7cfd 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -232,3 +232,7 @@ PAGURE_CI_SERVICES = [] # Boolean to turn on project being by default in the user's namespace USER_NAMESPACE = False + +# list of allowed tag colors +TAG_COLOR_LIST = ['DeepSkyBlue', 'red', 'maroon', 'SlateBlue', 'teal', 'green', + 'brown', 'orange', 'gray', 'purple', 'black'] diff --git a/pagure/forms.py b/pagure/forms.py index e5daf63..1283802 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -226,7 +226,7 @@ class RemoteRequestPullForm(RequestPullForm): class AddIssueTagForm(PagureForm): - ''' Form to add a comment to an issue. ''' + ''' Form to add a tag to an issue. ''' tag = wtforms.TextField( 'tag', [ @@ -235,6 +235,18 @@ class AddIssueTagForm(PagureForm): wtforms.validators.Length(max=255), ] ) + tag_color = wtforms.SelectField( + 'tag_color', + [wtforms.validators.Optional()], + choices=[] + ) + + def __init__(self, *args, **kwargs): + super(AddIssueTagForm, self).__init__(*args, **kwargs) + self.tag_color.choices = [ + (tag_color, tag_color) for tag_color in + pagure.APP.config['TAG_COLOR_LIST'] + ] class StatusForm(PagureForm): @@ -287,7 +299,6 @@ class NewTokenForm(PagureForm): ] - class UpdateIssueForm(PagureForm): ''' Form to add a comment to an issue. ''' tag = wtforms.TextField( diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 8de7e57..3498a2f 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -331,9 +331,21 @@ def add_tag_obj(session, obj, tags, user, ticketfolder): if known: continue - tagobj = get_tag(session, objtag) + if isinstance(obj, model.Project): + proj_id = obj.id + else: + proj_id = obj.project.id + tagobj = get_tag(session, objtag, proj_id) if not tagobj: - tagobj = model.Tag(tag=objtag) + if isinstance(obj, model.Project): + project_id = obj.id + else: + project_id = obj.project.id + + tagobj = model.Tag(tag=objtag, + tag_color="DeepSkyBlue", + project_id=project_id) + session.add(tagobj) session.flush() @@ -341,6 +353,8 @@ def add_tag_obj(session, obj, tags, user, ticketfolder): dbobjtag = model.TagIssue( issue_uid=obj.uid, tag=tagobj.tag, + tag_color=tagobj.tag_color, + tag_id=tagobj.tag_id ) if isinstance(obj, model.Project): dbobjtag = model.TagProject( @@ -641,17 +655,27 @@ def remove_tags(session, project, tags, ticketfolder, user): msgs = [] removed_tags = [] - if not issues: + tag_found = False + for tag in tags: + deltags = session.query(model.Tag).filter( + model.Tag.tag == tag).filter( + model.Tag.project_id == project.id).all() + for deltag in deltags: + tag_found = True + removed_tags.append(tag) + msgs.append('Removed tag: %s' % tag) + session.delete(deltag) + + if not tag_found: raise pagure.exceptions.PagureException( - 'No issue found with the tags: %s' % ', '.join(tags)) - else: + 'Tags not found: %s' % ', '.join(tags)) + + if issues is not None: for issue in issues: for issue_tag in issue.tags: if issue_tag.tag in tags: tag = issue_tag.tag - removed_tags.append(tag) session.delete(issue_tag) - msgs.append('Removed tag: %s' % tag) pagure.lib.git.update_git( issue, repo=issue.project, repofolder=ticketfolder) @@ -669,8 +693,7 @@ def remove_tags(session, project, tags, ticketfolder, user): return msgs -def remove_tags_obj( - session, obj, tags, ticketfolder, user): +def remove_tags_obj(session, obj, tags, ticketfolder, user): ''' Removes the specified tag(s) of a given object. ''' user_obj = get_user(session, user) @@ -708,68 +731,89 @@ def remove_tags_obj( return 'Removed tag: %s' % ', '.join(removed_tags) -def edit_issue_tags(session, project, old_tag, new_tag, ticketfolder, user): +def edit_issue_tags(session, project, old_tag, new_tag, old_tag_color, + new_tag_color, ticketfolder, user): ''' Removes the specified tag of a project. ''' user_obj = get_user(session, user) - if old_tag == new_tag: + if old_tag == new_tag and old_tag_color == new_tag_color: + # check for change raise pagure.exceptions.PagureException( - 'Old tag: "%s" is the same as new tag "%s", nothing to change' - % (old_tag, new_tag)) + 'No change. Old tag "%s(%s)" is the same as new tag "%s(%s)"' + % (old_tag, old_tag_color, new_tag, new_tag_color)) + elif old_tag != new_tag: + # Check if new tag already exists + existing_tag = session.query(model.Tag).filter( + model.Tag.tag == new_tag).filter( + model.Tag.project_id == project.id).all() + if existing_tag is not None and len(existing_tag) > 0: + raise pagure.exceptions.PagureException( + 'Can not rename a tag to an existing tag name: ' + new_tag) issues = search_issues(session, project, closed=False, tags=old_tag) issues.extend(search_issues(session, project, closed=True, tags=old_tag)) - msgs = [] - if not issues: - raise pagure.exceptions.PagureException( - 'No issue found with the tags: %s' % old_tag) - else: - tagobj = get_tag(session, new_tag) - if not tagobj: - tagobj = model.Tag(tag=new_tag) - session.add(tagobj) - session.flush() + # Grab the old tag (there can only be one), and remove it. + orig_tags = session.query(model.Tag).filter( + model.Tag.tag == old_tag).filter( + model.Tag.project_id == project.id).all() + + if orig_tags is not None and len(orig_tags) > 0: + session.delete(orig_tags[0]) + session.commit() + # Now, add the new tag since the old was removed + tagobj = model.Tag(tag=new_tag, tag_color=new_tag_color, + project_id=project.id) + session.add(tagobj) + session.flush() + + # Update the Issues tag list for issue in set(issues): - add = True - # Drop the old tag + # Drop the old tag from the issue cnt = 0 while cnt < len(issue.tags): issue_tag = issue.tags[cnt] if issue_tag.tag == old_tag: issue.tags.remove(issue_tag) cnt -= 1 - if issue_tag.tag == new_tag: - add = False cnt += 1 session.flush() - # Add the new one - if add: - issue_tag = model.TagIssue( - issue_uid=issue.uid, - tag=tagobj.tag - ) - session.add(issue_tag) - session.flush() + # Add the new one to the issue + issue_tag = model.TagIssue( + issue_uid=issue.uid, + tag=tagobj.tag, + tag_color=tagobj.tag_color, + tag_id=tagobj.tag_id + ) + issue.tags.append(issue_tag) + session.add(issue_tag) + session.flush() # Update the git version pagure.lib.git.update_git( issue, repo=issue.project, repofolder=ticketfolder) + else: + raise pagure.exceptions.PagureException( + 'Tag not found: %s' % old_tag) - msgs.append('Edited tag: %s to %s' % (old_tag, new_tag)) - pagure.lib.notify.log( - project, - topic='project.tag.edited', - msg=dict( - project=project.to_json(public=True), - old_tag=old_tag, - new_tag=new_tag, - agent=user_obj.username, - ), - redis=REDIS, - ) + msgs = [] + msgs.append('Edited tag: %s(%s) to %s(%s)' % + (old_tag, old_tag_color, new_tag, new_tag_color)) + pagure.lib.notify.log( + project, + topic='project.tag.edited', + msg=dict( + project=project.to_json(public=True), + old_tag=old_tag, + old_tag_color=old_tag_color, + new_tag=new_tag, + new_tag_color=new_tag_color, + agent=user_obj.username, + ), + redis=REDIS, + ) return msgs @@ -1338,6 +1382,11 @@ def new_pull_request(session, branch_from, return request +def new_tag(tag_name, tag_color, project_id): + ''' Return a new tag object ''' + return model.Tag(tag=tag_name, tag_color=tag_color, project_id=project_id) + + def edit_issue(session, issue, ticketfolder, user, title=None, content=None, status=None, close_status=None, priority=None, milestone=None, private=False): @@ -2021,11 +2070,9 @@ def get_tags_of_project(session, project, pattern=None): query = session.query( model.Tag ).filter( - model.Tag.tag == model.TagIssue.tag + model.Tag.tag != "" ).filter( - model.TagIssue.issue_uid == model.Issue.uid - ).filter( - model.Issue.project_id == project.id + model.Tag.project_id == project.id ).order_by( model.Tag.tag ) @@ -2038,13 +2085,15 @@ def get_tags_of_project(session, project, pattern=None): return query.all() -def get_tag(session, tag): +def get_tag(session, tag, project_id): ''' Returns a Tag object for the given tag text. ''' query = session.query( model.Tag ).filter( model.Tag.tag == tag + ).filter( + model.Tag.project_id == project_id ) return query.first() diff --git a/pagure/lib/model.py b/pagure/lib/model.py index c44b604..1aaf1b6 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1034,9 +1034,18 @@ class Tag(BASE): __tablename__ = 'tags' - tag = sa.Column(sa.String(255), primary_key=True) + tag_id = sa.Column(sa.Integer, primary_key=True) + tag = sa.Column(sa.String(255), nullable=False) + tag_color = sa.Column(sa.String(25), default="DeepSkyBlue") date_created = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow) + project_id = sa.Column(sa.Integer, nullable=False) + + def __init__(self, tag=None, project_id=None, tag_color="DeepSkyBlue"): + # Create our unique tag identifier + self.tag = tag + self.project_id = project_id + self.tag_color = tag_color class TagIssue(BASE): @@ -1047,10 +1056,10 @@ class TagIssue(BASE): __tablename__ = 'tags_issues' - tag = sa.Column( - sa.String(255), + tag_id = sa.Column( + sa.Integer, sa.ForeignKey( - 'tags.tag', ondelete='CASCADE', onupdate='CASCADE', + 'tags.tag_id', ondelete='CASCADE', onupdate='CASCADE', ), primary_key=True) issue_uid = sa.Column( @@ -1059,6 +1068,16 @@ class TagIssue(BASE): 'issues.uid', ondelete='CASCADE', onupdate='CASCADE', ), primary_key=True) + tag_color = sa.Column( + sa.String(25), + sa.ForeignKey( + 'tags.tag_color', ondelete='CASCADE', onupdate='CASCADE', + )) + tag = sa.Column( + sa.String(255), + sa.ForeignKey( + 'tags.tag', ondelete='CASCADE', onupdate='CASCADE', + )) date_created = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow) diff --git a/pagure/templates/edit_tag.html b/pagure/templates/edit_tag.html index 254fbcf..0cc503a 100644 --- a/pagure/templates/edit_tag.html +++ b/pagure/templates/edit_tag.html @@ -8,25 +8,41 @@ {% block repo %} -

Edit tag: {{ edit_tag }}

-
-
- -

Enter in the field below the new name for the tag: "{{ edit_tag }}"

- - {{ render_field_in_row(form.tag) }} -
-

- - - {{ form.csrf_token }} -

-
+
+
+
+ Edit tag: {{ edit_tag }} +
+
+ {{ form.csrf_token }} +
+
+ +
+
+ +
+
+

+ + + {{ form.csrf_token }} +

+
+
+
- {% endblock %} diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html index ca1f4cf..4e5a186 100644 --- a/pagure/templates/issue.html +++ b/pagure/templates/issue.html @@ -159,7 +159,7 @@

{% for tag in issue.tags %} -
- {% for tag in tags %} + {% for tag in tag_list %} {% endfor %} {% endif %} - {% for tag in issue.tags%} - {{tag.tag}} + {% for tag in issue.tags %} + {{tag.tag}} {% endfor%} diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index 5a9fd5a..96a1924 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -750,8 +750,7 @@

- Here below is the list of tags associated with one or more issue of - the project. + Here is the list of tags associated with this project.

+ + {{ tag_form.csrf_token }} +
+ +
+
+
+
+ + Add New Tag + +
+
+
+
+ +
+
+
+ {% endif %} @@ -966,6 +995,42 @@ $('#default_close_status').click(function(e) { }); {% endif %} + +var first_new_tag = 1; +$('#new_tag').click(function(e) { + console.log('new tag'); + console.log($('#tagcolor')); + if (first_new_tag == 1){ + // Only display the Tag row the first time Add New Tag is clicked + $('#tagcolor').append( + '
\ +
\ + New Tag\ +
\ +
\ + Tag Color\ +
\ +
'); + first_new_tag = 0; + } + $('#tagcolor').append( + '
\ +
\ + \ +
\ +
\ + \ +
\ +
' + ); +}); + $('.extend-form').click(function(e) { const tgt = $(this).attr('data-target'); let form = $(tgt + ' > div:last-child').clone(); diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 537c968..c931ed5 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -374,16 +374,25 @@ def edit_tag(repo, tag, username=None, namespace=None): flask.abort(404, 'No issue tracker found for this project') tags = pagure.lib.get_tags_of_project(SESSION, repo) - - if not tags or tag not in [t.tag for t in tags]: + if not tags: + flask.abort(404, 'Project has no tags to edit') + + # Check the tag exists, and get its old/original color + found = False + for t in tags: + if t.tag == tag: + old_tag_color = t.tag_color + found = True + break + if not found: flask.abort(404, 'Tag %s not found in this project' % tag) form = pagure.forms.AddIssueTagForm() if form.validate_on_submit(): new_tag = form.tag.data - + new_tag_color = form.tag_color.data msgs = pagure.lib.edit_issue_tags( - SESSION, repo, tag, new_tag, + SESSION, repo, tag, new_tag, old_tag_color, new_tag_color, user=flask.g.fas_user.username, ticketfolder=APP.config['TICKETS_FOLDER'] ) @@ -407,9 +416,76 @@ def edit_tag(repo, tag, username=None, namespace=None): username=username, repo=repo, edit_tag=tag, + tag_color=old_tag_color, + color_list=pagure.APP.config['TAG_COLOR_LIST'] ) +@APP.route('//update/tags', methods=['POST']) +@APP.route('///update/tags', methods=['POST']) +@login_required +def update_tags(repo, username=None, namespace=None): + """ Update the tags of a project. + """ + + repo = flask.g.repo + + if not repo.settings.get('issue_tracker', True): + flask.abort(404, 'No issue tracker found for this project') + + if not flask.g.repo_admin: + flask.abort( + 403, + 'You are not allowed to change the settings for this project') + + form = pagure.forms.ConfirmationForm() + + error = False + if form.validate_on_submit(): + tag_names = flask.request.form.getlist('tag') + tag_colors = flask.request.form.getlist('tag_color_select') + + tags = [] + colors = [] + for t in range(len(tag_names)): + if tag_names[t] == "": + # Blank field, ignore + continue + tags.append(tag_names[t].strip()) + colors.append(tag_colors[t].strip()) + + if len(tags) != len(colors): + flask.flash( + 'tags and tag colors are not of the same length', 'error') + error = True + + for tag in tags: + if tag.strip() and tags.count(tag) != 1: + flask.flash( + 'Tag %s is present %s times' % ( + tag, tags.count(tag) + ), + 'error') + error = True + break + + if not error: + for cnt in range(len(tags)): + new_tag = pagure.lib.new_tag(tags[cnt].strip(), + colors[cnt], repo.id) + try: + SESSION.add(new_tag) + SESSION.commit() + flask.flash('Tags updated') + except SQLAlchemyError as err: # pragma: no cover + SESSION.rollback() + flask.flash(str(err), 'error') + + return flask.redirect(flask.url_for( + 'view_settings', username=username, repo=repo.name, + namespace=namespace)) + + @APP.route('//droptag/', methods=['POST']) @APP.route('///droptag/', methods=['POST']) @APP.route('/fork///droptag/', methods=['POST']) diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 60d354c..9e80472 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -1080,6 +1080,7 @@ def view_settings(repo, username=None, namespace=None): tags=tags, plugins=plugins, branchname=branchname, + color_list=pagure.APP.config['TAG_COLOR_LIST'] ) diff --git a/tests/test_pagure_flask_api.py b/tests/test_pagure_flask_api.py index 44fffca..cc2269b 100644 --- a/tests/test_pagure_flask_api.py +++ b/tests/test_pagure_flask_api.py @@ -77,13 +77,14 @@ class PagureFlaskApitests(tests.Modeltests): self.session.add(item) self.session.commit() item = pagure.lib.model.Tag( - tag='tag1', + tag='tag1', tag_color='DeepBlueSky', project_id=1, ) self.session.add(item) self.session.commit() item = pagure.lib.model.TagIssue( tag='tag1', issue_uid='foobar', + tag_id=tag.tag_id ) self.session.add(item) self.session.commit() diff --git a/tests/test_pagure_flask_ui_issues.py b/tests/test_pagure_flask_ui_issues.py index 878cab6..ea8b874 100644 --- a/tests/test_pagure_flask_ui_issues.py +++ b/tests/test_pagure_flask_ui_issues.py @@ -1512,31 +1512,26 @@ class PagureFlaskIssuestests(tests.Modeltests): output = self.app.get('/test/tag/tag1/edit') self.assertEqual(output.status_code, 200) - self.assertTrue('

Edit tag: tag1

' in output.data) - self.assertTrue( - '

Enter in the field below the new name for the tag: ' - '"tag1"

' in output.data) + self.assertTrue('Edit tag: tag1' in output.data) csrf_token = output.data.split( 'name="csrf_token" type="hidden" value="')[1].split('">')[0] - data = {'tag': 'tag2'} + data = {'tag': 'tag2', + 'tag_color': 'DeepSkyBlue'} output = self.app.post('/test/tag/tag1/edit', data=data) self.assertEqual(output.status_code, 200) - self.assertTrue('

Edit tag: tag1

' in output.data) - self.assertTrue( - '

Enter in the field below the new name for the tag: ' - '"tag1"

' in output.data) + self.assertTrue('Edit tag: tag1' in output.data) data['csrf_token'] = csrf_token output = self.app.post( '/test/tag/tag1/edit', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( - 'Settings - test - Pagure', output.data) + 'Settings - test - Pagure', output.data) self.assertIn( - '\n Edited tag: tag1 to tag2', + '\n Edited tag: tag1(DeepSkyBlue) to tag2(DeepSkyBlue)', output.data) # After edit, list tags diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index cdfd9c5..7677028 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -365,7 +365,6 @@ class PagureLibtests(tests.Modeltests): self.test_add_tag_obj() repo = pagure.lib.get_project(self.session, 'test') issue = pagure.lib.search_issues(self.session, repo, issueid=1) - self.assertRaises( pagure.exceptions.PagureException, pagure.lib.remove_tags, @@ -421,7 +420,9 @@ class PagureLibtests(tests.Modeltests): session=self.session, project=repo, old_tag='foo', + old_tag_color='DeepSkyBlue', new_tag='bar', + new_tag_color='black', user='pingou', ticketfolder=None, ) @@ -430,12 +431,14 @@ class PagureLibtests(tests.Modeltests): session=self.session, project=repo, old_tag='tag1', + old_tag_color='DeepSkyBlue', new_tag='tag2', + new_tag_color='black', user='pingou', ticketfolder=None, ) self.session.commit() - self.assertEqual(msgs, ['Edited tag: tag1 to tag2']) + self.assertEqual(msgs, ['Edited tag: tag1(DeepSkyBlue) to tag2(black)']) # Add a new tag msg = pagure.lib.add_tag_obj( @@ -448,29 +451,33 @@ class PagureLibtests(tests.Modeltests): self.assertEqual(msg, 'Tag added: tag3') self.assertEqual([tag.tag for tag in issue.tags], ['tag2', 'tag3']) - # Rename an existing tag into another existing one - msgs = pagure.lib.edit_issue_tags( + # Attempt to rename an existing tag into another existing one + self.assertRaises( + pagure.exceptions.PagureException, + pagure.lib.edit_issue_tags, session=self.session, project=repo, old_tag='tag2', + old_tag_color='black', new_tag='tag3', + new_tag_color='red', user='pingou', ticketfolder=None, ) - self.session.commit() - self.assertEqual(msgs, ['Edited tag: tag2 to tag3']) - self.assertEqual([tag.tag for tag in issue.tags], ['tag3']) - self.assertRaises( - pagure.exceptions.PagureException, - pagure.lib.edit_issue_tags, + # Rename an existing tag + msgs = pagure.lib.edit_issue_tags( session=self.session, project=repo, old_tag='tag2', - new_tag='tag2', + old_tag_color='black', + new_tag='tag4', + new_tag_color='purple', user='pingou', ticketfolder=None, ) + self.session.commit() + self.assertEqual(msgs, ['Edited tag: tag2(black) to tag4(purple)']) @patch('pagure.lib.git.update_git') @patch('pagure.lib.notify.send_email') diff --git a/tests/test_pagure_lib_model.py b/tests/test_pagure_lib_model.py index 7457430..9708b4a 100644 --- a/tests/test_pagure_lib_model.py +++ b/tests/test_pagure_lib_model.py @@ -132,17 +132,19 @@ class PagureLibModeltests(tests.Modeltests): issues = pagure.lib.search_issues(self.session, repo) self.assertEqual(len(issues), 1) - item = pagure.lib.model.Tag(tag='foo') + item = pagure.lib.model.Tag(tag='foo', tag_color='DeepSkyBlue', + project_id=repo.id) self.session.add(item) self.session.commit() item = pagure.lib.model.TagIssue( + issue=issues[0], issue_uid=issues[0].uid, tag='foo', + tag_id=item.tag_id ) self.session.add(item) self.session.commit() - self.assertEqual(str(item), 'TagIssue(issue:1, tag:foo)') From 16035cb20de501e6f10d36ec5ca0a3e5e4a8f1b9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 2/9] Create the tags_colored and tags_issues_colored tables in the DB These will allow an easier migration from the current tag model to the new one as, as it is now, it would be near impossible to migrate. --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 1aaf1b6..9ae2efe 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -708,6 +708,13 @@ class Issue(BASE): backref="children", ) + tags = relation( + "TagColored", + secondary="tags_issues_colored", + primaryjoin="issues.c.uid==tags_issues_colored.c.issue_uid", + secondaryjoin="tags_issues_colored.c.tag_id==tags_colored.c.id", + ) + def __repr__(self): return 'Issue(%s, project:%s, user:%s, title:%s)' % ( self.id, self.project.name, self.user.user, self.title @@ -1034,18 +1041,9 @@ class Tag(BASE): __tablename__ = 'tags' - tag_id = sa.Column(sa.Integer, primary_key=True) - tag = sa.Column(sa.String(255), nullable=False) - tag_color = sa.Column(sa.String(25), default="DeepSkyBlue") + tag = sa.Column(sa.String(255), primary_key=True) date_created = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow) - project_id = sa.Column(sa.Integer, nullable=False) - - def __init__(self, tag=None, project_id=None, tag_color="DeepSkyBlue"): - # Create our unique tag identifier - self.tag = tag - self.project_id = project_id - self.tag_color = tag_color class TagIssue(BASE): @@ -1056,10 +1054,10 @@ class TagIssue(BASE): __tablename__ = 'tags_issues' - tag_id = sa.Column( - sa.Integer, + tag = sa.Column( + sa.String(255), sa.ForeignKey( - 'tags.tag_id', ondelete='CASCADE', onupdate='CASCADE', + 'tags.tag', ondelete='CASCADE', onupdate='CASCADE', ), primary_key=True) issue_uid = sa.Column( @@ -1068,29 +1066,86 @@ class TagIssue(BASE): 'issues.uid', ondelete='CASCADE', onupdate='CASCADE', ), primary_key=True) - tag_color = sa.Column( - sa.String(25), - sa.ForeignKey( - 'tags.tag_color', ondelete='CASCADE', onupdate='CASCADE', - )) - tag = sa.Column( - sa.String(255), - sa.ForeignKey( - 'tags.tag', ondelete='CASCADE', onupdate='CASCADE', - )) date_created = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow) issue = relation( 'Issue', foreign_keys=[issue_uid], remote_side=[Issue.uid], backref=backref( - 'tags', cascade="delete, delete-orphan", single_parent=True) + 'old_tags', cascade="delete, delete-orphan", single_parent=True) ) def __repr__(self): return 'TagIssue(issue:%s, tag:%s)' % (self.issue.id, self.tag) + +class TagColored(BASE): + """ Stores the colored tags. + + Table -- tags_colored + """ + + __tablename__ = 'tags_colored' + + id = sa.Column(sa.Integer, primary_key=True) + tag = sa.Column(sa.String(255), nullable=False) + project_id = sa.Column( + sa.Integer, + sa.ForeignKey( + 'projects.id', ondelete='CASCADE', onupdate='CASCADE', + ), + nullable=False, + ) + tag_color = sa.Column(sa.String(25), default="DeepSkyBlue") + date_created = sa.Column(sa.DateTime, nullable=False, + default=datetime.datetime.utcnow) + + __table_args__ = (sa.UniqueConstraint('project_id', 'tag'),) + + project = relation( + 'Project', foreign_keys=[project_id], remote_side=[Project.id], + ) + + def __repr__(self): + return 'TagColored(tag:%s, color:%s)' % (self.tag, self.tag_color) + + +class TagIssueColored(BASE): + """ Stores the colored tag associated with an issue. + + Table -- tags_issues_colored + """ + + __tablename__ = 'tags_issues_colored' + + tag_id = sa.Column( + sa.Integer, + sa.ForeignKey( + 'tags_colored.id', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True) + issue_uid = sa.Column( + sa.String(32), + sa.ForeignKey( + 'issues.uid', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True) + date_created = sa.Column(sa.DateTime, nullable=False, + default=datetime.datetime.utcnow) + + issue = relation( + 'Issue', foreign_keys=[issue_uid], remote_side=[Issue.uid], + ) + tag = relation( + 'TagColored', foreign_keys=[tag_id], remote_side=[TagColored.id], + ) + + def __repr__(self): + return 'TagIssueColored(issue:%s, tag:%s, project:%s)' % ( + self.issue.id, self.tag.tag, self.tag.project.fullname) + + class TagProject(BASE): """ Stores the tag associated with a project. From 2bc953adca036813b1a1bdf4d7267894864556e3 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 3/9] Rework the colored tag feature This commit does a few thing: - Drop the hard-coded list of color - Instead of this list, offer the user to pick any color they wishes to use in a color-picker widget (html5 powered) - Fix editing a tag - simplify the logic while at it so that it runs an update query instead of having to manipulate the issues (removing the old tag then adding the new one...) - Fix updating a tag - Fix deleting a tag - Small code clean here and there to avoid duplication (most around the using the get_tag() method) - Adjust pagure.lib for the change in the DB model to use TagColored where we were using Tag --- diff --git a/pagure/default_config.py b/pagure/default_config.py index 7fb7cfd..4dc234e 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -195,7 +195,7 @@ APPLICATION_ROOT = '/' BLACKLISTED_PROJECTS = [ 'static', 'pv', 'releases', 'new', 'api', 'settings', 'search', 'fork', 'logout', 'login', 'user', 'users', 'groups', 'projects', 'ssh_info', - 'issues', 'pull-requests', 'commits', 'tree', 'forks', 'admin', + 'issues', 'pull-requests', 'commits', 'tree', 'forks', 'admin', ] # List of prefix allowed in project names @@ -232,7 +232,3 @@ PAGURE_CI_SERVICES = [] # Boolean to turn on project being by default in the user's namespace USER_NAMESPACE = False - -# list of allowed tag colors -TAG_COLOR_LIST = ['DeepSkyBlue', 'red', 'maroon', 'SlateBlue', 'teal', 'green', - 'brown', 'orange', 'gray', 'purple', 'black'] diff --git a/pagure/forms.py b/pagure/forms.py index 1283802..576896f 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -225,8 +225,8 @@ class RemoteRequestPullForm(RequestPullForm): ) -class AddIssueTagForm(PagureForm): - ''' Form to add a tag to an issue. ''' +class DeleteIssueTagForm(PagureForm): + ''' Form to remove a tag to from a project. ''' tag = wtforms.TextField( 'tag', [ @@ -235,19 +235,15 @@ class AddIssueTagForm(PagureForm): wtforms.validators.Length(max=255), ] ) - tag_color = wtforms.SelectField( + + +class AddIssueTagForm(DeleteIssueTagForm): + ''' Form to add a tag to a project. ''' + tag_color = wtforms.TextField( 'tag_color', - [wtforms.validators.Optional()], - choices=[] + [wtforms.validators.Required()], ) - def __init__(self, *args, **kwargs): - super(AddIssueTagForm, self).__init__(*args, **kwargs) - self.tag_color.choices = [ - (tag_color, tag_color) for tag_color in - pagure.APP.config['TAG_COLOR_LIST'] - ] - class StatusForm(PagureForm): ''' Form to add/change the status of an issue. ''' diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 3498a2f..222c334 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -336,32 +336,33 @@ def add_tag_obj(session, obj, tags, user, ticketfolder): else: proj_id = obj.project.id tagobj = get_tag(session, objtag, proj_id) - if not tagobj: - if isinstance(obj, model.Project): - project_id = obj.id - else: - project_id = obj.project.id - tagobj = model.Tag(tag=objtag, - tag_color="DeepSkyBlue", - project_id=project_id) + if obj.isa == 'project': + if not tagobj: + tagobj = model.Tag(tag=objtag) - session.add(tagobj) - session.flush() + session.add(tagobj) + session.flush() - if isinstance(obj, model.Issue): - dbobjtag = model.TagIssue( - issue_uid=obj.uid, - tag=tagobj.tag, - tag_color=tagobj.tag_color, - tag_id=tagobj.tag_id - ) - if isinstance(obj, model.Project): dbobjtag = model.TagProject( project_id=obj.id, tag=tagobj.tag, ) + else: + if not tagobj: + tagobj = model.TagColored( + tag=objtag, + project_id=proj_id + ) + session.add(tagobj) + session.flush() + + dbobjtag = model.TagIssueColored( + issue_uid=obj.uid, + tag_id=tagobj.id + ) + session.add(dbobjtag) # Make sure we won't have SQLAlchemy error before we continue session.flush() @@ -657,14 +658,12 @@ def remove_tags(session, project, tags, ticketfolder, user): removed_tags = [] tag_found = False for tag in tags: - deltags = session.query(model.Tag).filter( - model.Tag.tag == tag).filter( - model.Tag.project_id == project.id).all() - for deltag in deltags: + tagobj = get_tag(session, tag, project.id) + if tagobj: tag_found = True removed_tags.append(tag) msgs.append('Removed tag: %s' % tag) - session.delete(deltag) + session.delete(tagobj) if not tag_found: raise pagure.exceptions.PagureException( @@ -731,82 +730,68 @@ def remove_tags_obj(session, obj, tags, ticketfolder, user): return 'Removed tag: %s' % ', '.join(removed_tags) -def edit_issue_tags(session, project, old_tag, new_tag, old_tag_color, - new_tag_color, ticketfolder, user): +def edit_issue_tags( + session, project, old_tag, new_tag, + new_tag_color, ticketfolder, user): ''' Removes the specified tag of a project. ''' user_obj = get_user(session, user) + old_tag_name = old_tag - if old_tag == new_tag and old_tag_color == new_tag_color: + if not isinstance(old_tag, model.TagColored): + old_tag = get_tag(session, old_tag_name, project.id) + + if not old_tag: + raise pagure.exceptions.PagureException( + 'No tag "%s" found related to this project' % (old_tag_name)) + + old_tag_name = old_tag.tag + old_tag_color = old_tag.tag_color + if old_tag.tag == new_tag and old_tag_color == new_tag_color: # check for change raise pagure.exceptions.PagureException( 'No change. Old tag "%s(%s)" is the same as new tag "%s(%s)"' % (old_tag, old_tag_color, new_tag, new_tag_color)) - elif old_tag != new_tag: + elif old_tag.tag != new_tag: # Check if new tag already exists - existing_tag = session.query(model.Tag).filter( - model.Tag.tag == new_tag).filter( - model.Tag.project_id == project.id).all() - if existing_tag is not None and len(existing_tag) > 0: + existing_tag = get_tag(session, new_tag, project.id) + if existing_tag: raise pagure.exceptions.PagureException( - 'Can not rename a tag to an existing tag name: ' + new_tag) - - issues = search_issues(session, project, closed=False, tags=old_tag) - issues.extend(search_issues(session, project, closed=True, tags=old_tag)) - - # Grab the old tag (there can only be one), and remove it. - orig_tags = session.query(model.Tag).filter( - model.Tag.tag == old_tag).filter( - model.Tag.project_id == project.id).all() - - if orig_tags is not None and len(orig_tags) > 0: - session.delete(orig_tags[0]) - session.commit() - - # Now, add the new tag since the old was removed - tagobj = model.Tag(tag=new_tag, tag_color=new_tag_color, - project_id=project.id) - session.add(tagobj) - session.flush() - - # Update the Issues tag list - for issue in set(issues): - # Drop the old tag from the issue - cnt = 0 - while cnt < len(issue.tags): - issue_tag = issue.tags[cnt] - if issue_tag.tag == old_tag: - issue.tags.remove(issue_tag) - cnt -= 1 - cnt += 1 - session.flush() + 'Can not rename a tag to an existing tag name: %s' % new_tag) - # Add the new one to the issue - issue_tag = model.TagIssue( - issue_uid=issue.uid, - tag=tagobj.tag, - tag_color=tagobj.tag_color, - tag_id=tagobj.tag_id - ) - issue.tags.append(issue_tag) - session.add(issue_tag) - session.flush() + session.query( + model.TagColored + ).filter( + model.TagColored.tag == old_tag.tag + ).filter( + model.TagColored.project_id == project.id + ).update( + { + model.TagColored.tag: new_tag, + model.TagColored.tag_color: new_tag_color + } + ) - # Update the git version - pagure.lib.git.update_git( - issue, repo=issue.project, repofolder=ticketfolder) - else: - raise pagure.exceptions.PagureException( - 'Tag not found: %s' % old_tag) + issues = session.query( + model.Issue + ).filter( + model.TagIssueColored.tag_id == old_tag.id + ).filter( + model.TagIssueColored.issue_uid == model.Issue.uid + ).all() + for issue in issues: + # Update the git version + pagure.lib.git.update_git( + issue, repo=issue.project, repofolder=ticketfolder) msgs = [] msgs.append('Edited tag: %s(%s) to %s(%s)' % - (old_tag, old_tag_color, new_tag, new_tag_color)) + (old_tag_name, old_tag_color, new_tag, new_tag_color)) pagure.lib.notify.log( project, topic='project.tag.edited', msg=dict( project=project.to_json(public=True), - old_tag=old_tag, + old_tag=old_tag.tag, old_tag_color=old_tag_color, new_tag=new_tag, new_tag_color=new_tag_color, @@ -1382,9 +1367,17 @@ def new_pull_request(session, branch_from, return request -def new_tag(tag_name, tag_color, project_id): +def new_tag(session, tag_name, tag_color, project_id): ''' Return a new tag object ''' - return model.Tag(tag=tag_name, tag_color=tag_color, project_id=project_id) + tagobj = model.TagColored( + tag=tag_name, + tag_color=tag_color, + project_id=project_id + ) + session.add(tagobj) + session.flush() + + return tagobj def edit_issue(session, issue, ticketfolder, user, @@ -1917,9 +1910,11 @@ def search_issues( ).filter( model.Issue.project_id == repo.id ).filter( - model.Issue.uid == model.TagIssue.issue_uid + model.Issue.uid == model.TagIssueColored.issue_uid ).filter( - model.TagIssue.tag.in_(ytags) + model.TagIssueColored.tag_id == model.TagColored.id + ).filter( + model.TagColored.tag.in_(ytags) ) if notags: sub_q3 = session.query( @@ -1927,9 +1922,11 @@ def search_issues( ).filter( model.Issue.project_id == repo.id ).filter( - model.Issue.uid == model.TagIssue.issue_uid + model.Issue.uid == model.TagIssueColored.issue_uid + ).filter( + model.TagIssueColored.tag_id == model.TagColored.id ).filter( - model.TagIssue.tag.in_(notags) + model.TagColored.tag.in_(notags) ) # Adjust the main query based on the parameters specified if ytags and not notags: @@ -2068,18 +2065,18 @@ def get_tags_of_project(session, project, pattern=None): ''' Returns the list of tags associated with the issues of a project. ''' query = session.query( - model.Tag + model.TagColored ).filter( - model.Tag.tag != "" + model.TagColored.tag != "" ).filter( - model.Tag.project_id == project.id + model.TagColored.project_id == project.id ).order_by( - model.Tag.tag + model.TagColored.tag ) if pattern: query = query.filter( - model.Tag.tag.ilike(pattern.replace('*', '%')) + model.TagColored.tag.ilike(pattern.replace('*', '%')) ) return query.all() @@ -2089,11 +2086,11 @@ def get_tag(session, tag, project_id): ''' Returns a Tag object for the given tag text. ''' query = session.query( - model.Tag + model.TagColored ).filter( - model.Tag.tag == tag + model.TagColored.tag == tag ).filter( - model.Tag.project_id == project_id + model.TagColored.project_id == project_id ) return query.first() diff --git a/pagure/templates/edit_tag.html b/pagure/templates/edit_tag.html index 0cc503a..6775017 100644 --- a/pagure/templates/edit_tag.html +++ b/pagure/templates/edit_tag.html @@ -1,7 +1,7 @@ {% extends "repo_master.html" %} -{% from "_formhelper.html" import render_field_in_row %} +{% from "_formhelper.html" import render_bootstrap_field %} -{% block title %}Edit tag: {{ tag }} - {{ +{% block title %}Edit tag: {{ tagname }} - {{ repo.namespace + '/' if repo.namespace }}{{ repo.name }}{% endblock %} {% set tag = "home" %} @@ -9,40 +9,46 @@ {% block repo %}
-
-
-
- Edit tag: {{ edit_tag }} -
-
+
+
+ Edit tag: {{ tagname }} +
+
+ - {{ form.csrf_token }} -
-
- -
-
- -
-
-

- - + tag=tagname, + namespace=repo.namespace) }}" > {{ form.csrf_token }} -

+ {{ render_bootstrap_field(form.tag) }} +
+ {% set formclasses = "form-control c-select"%} + {% if form.tag_color.errors %} {% set formclasses = formclasses + " form-control-error" %} {% endif %} + + {{ form.tag_color.label }} + + +
+ {% if form.tag_color.errors %} + + + {% for error in form.tag_color.errors %} + {{ error }}  + {% endfor %} + + + {% endif %} +
+
+ +
+
{% endblock %} diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index 96a1924..c28f43d 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -785,8 +785,7 @@ repo=repo.name, username=username, namespace=repo.namespace, - tag=tag.tag, - tag_color=tag.tag_color) }}"> + tag=tag.tag) }}"> @@ -1020,12 +1019,8 @@ $('#new_tag').click(function(e) { value="" size="3" class="form-control"/>\ \
\ - \ + \
\ ' ); diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index c931ed5..56147ea 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -378,21 +378,21 @@ def edit_tag(repo, tag, username=None, namespace=None): flask.abort(404, 'Project has no tags to edit') # Check the tag exists, and get its old/original color - found = False - for t in tags: - if t.tag == tag: - old_tag_color = t.tag_color - found = True - break - if not found: + tagobj = pagure.lib.get_tag(SESSION, tag, repo.id) + if not tagobj: flask.abort(404, 'Tag %s not found in this project' % tag) form = pagure.forms.AddIssueTagForm() if form.validate_on_submit(): new_tag = form.tag.data new_tag_color = form.tag_color.data + msgs = pagure.lib.edit_issue_tags( - SESSION, repo, tag, new_tag, old_tag_color, new_tag_color, + SESSION, + repo, + tagobj, + new_tag, + new_tag_color, user=flask.g.fas_user.username, ticketfolder=APP.config['TICKETS_FOLDER'] ) @@ -409,15 +409,16 @@ def edit_tag(repo, tag, username=None, namespace=None): return flask.redirect(flask.url_for( '.view_settings', repo=repo.name, username=username, namespace=repo.namespace)) + elif flask.request.method == 'GET': + form.tag_color.data = tagobj.tag_color + form.tag.data = tag return flask.render_template( 'edit_tag.html', - form=form, username=username, repo=repo, - edit_tag=tag, - tag_color=old_tag_color, - color_list=pagure.APP.config['TAG_COLOR_LIST'] + form=form, + tagname=tag, ) @@ -443,7 +444,7 @@ def update_tags(repo, username=None, namespace=None): error = False if form.validate_on_submit(): tag_names = flask.request.form.getlist('tag') - tag_colors = flask.request.form.getlist('tag_color_select') + tag_colors = flask.request.form.getlist('tag_color') tags = [] colors = [] @@ -471,10 +472,12 @@ def update_tags(repo, username=None, namespace=None): if not error: for cnt in range(len(tags)): - new_tag = pagure.lib.new_tag(tags[cnt].strip(), - colors[cnt], repo.id) try: - SESSION.add(new_tag) + pagure.lib.new_tag( + SESSION, + tags[cnt].strip(), + colors[cnt], + repo.id) SESSION.commit() flask.flash('Tags updated') except SQLAlchemyError as err: # pragma: no cover @@ -505,7 +508,7 @@ def remove_tag(repo, username=None, namespace=None): if not repo.settings.get('issue_tracker', True): flask.abort(404, 'No issue tracker found for this project') - form = pagure.forms.AddIssueTagForm() + form = pagure.forms.DeleteIssueTagForm() if form.validate_on_submit(): tags = form.tag.data tags = [tag.strip() for tag in tags.split(',')] diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 9e80472..60d354c 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -1080,7 +1080,6 @@ def view_settings(repo, username=None, namespace=None): tags=tags, plugins=plugins, branchname=branchname, - color_list=pagure.APP.config['TAG_COLOR_LIST'] ) From a8f6829009449a0a93490f4eb04fb31330dfb02a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 4/9] Adjust unit-tests for the changes made to the colored tags feature --- diff --git a/tests/test_pagure_flask_api.py b/tests/test_pagure_flask_api.py index cc2269b..c912d38 100644 --- a/tests/test_pagure_flask_api.py +++ b/tests/test_pagure_flask_api.py @@ -76,15 +76,14 @@ class PagureFlaskApitests(tests.Modeltests): ) self.session.add(item) self.session.commit() - item = pagure.lib.model.Tag( + item = pagure.lib.model.TagColored( tag='tag1', tag_color='DeepBlueSky', project_id=1, ) self.session.add(item) self.session.commit() - item = pagure.lib.model.TagIssue( - tag='tag1', + item = pagure.lib.model.TagIssueColored( issue_uid='foobar', - tag_id=tag.tag_id + tag_id=item.id ) self.session.add(item) self.session.commit() diff --git a/tests/test_pagure_flask_ui_issues.py b/tests/test_pagure_flask_ui_issues.py index ea8b874..6e79890 100644 --- a/tests/test_pagure_flask_ui_issues.py +++ b/tests/test_pagure_flask_ui_issues.py @@ -1531,7 +1531,8 @@ class PagureFlaskIssuestests(tests.Modeltests): self.assertIn( 'Settings - test - Pagure', output.data) self.assertIn( - '\n Edited tag: tag1(DeepSkyBlue) to tag2(DeepSkyBlue)', + '\n ' + 'Edited tag: tag1(DeepSkyBlue) to tag2(DeepSkyBlue)', output.data) # After edit, list tags diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 7677028..3ea6706 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -420,7 +420,18 @@ class PagureLibtests(tests.Modeltests): session=self.session, project=repo, old_tag='foo', - old_tag_color='DeepSkyBlue', + new_tag='bar', + new_tag_color='black', + user='pingou', + ticketfolder=None, + ) + + self.assertRaises( + pagure.exceptions.PagureException, + pagure.lib.edit_issue_tags, + session=self.session, + project=repo, + old_tag=None, new_tag='bar', new_tag_color='black', user='pingou', @@ -431,14 +442,16 @@ class PagureLibtests(tests.Modeltests): session=self.session, project=repo, old_tag='tag1', - old_tag_color='DeepSkyBlue', new_tag='tag2', new_tag_color='black', user='pingou', ticketfolder=None, ) self.session.commit() - self.assertEqual(msgs, ['Edited tag: tag1(DeepSkyBlue) to tag2(black)']) + self.assertEqual( + msgs, + ['Edited tag: tag1(DeepSkyBlue) to tag2(black)'] + ) # Add a new tag msg = pagure.lib.add_tag_obj( @@ -458,7 +471,6 @@ class PagureLibtests(tests.Modeltests): session=self.session, project=repo, old_tag='tag2', - old_tag_color='black', new_tag='tag3', new_tag_color='red', user='pingou', @@ -470,7 +482,6 @@ class PagureLibtests(tests.Modeltests): session=self.session, project=repo, old_tag='tag2', - old_tag_color='black', new_tag='tag4', new_tag_color='purple', user='pingou', diff --git a/tests/test_pagure_lib_model.py b/tests/test_pagure_lib_model.py index 9708b4a..696ea42 100644 --- a/tests/test_pagure_lib_model.py +++ b/tests/test_pagure_lib_model.py @@ -132,21 +132,43 @@ class PagureLibModeltests(tests.Modeltests): issues = pagure.lib.search_issues(self.session, repo) self.assertEqual(len(issues), 1) - item = pagure.lib.model.Tag(tag='foo', tag_color='DeepSkyBlue', - project_id=repo.id) + item = pagure.lib.model.Tag(tag='foo') self.session.add(item) self.session.commit() item = pagure.lib.model.TagIssue( - issue=issues[0], issue_uid=issues[0].uid, - tag='foo', - tag_id=item.tag_id + tag='foo' ) self.session.add(item) self.session.commit() self.assertEqual(str(item), 'TagIssue(issue:1, tag:foo)') + def test_tagissuecolor__repr__(self): + """ Test the TagIssue.__repr__ function of pagure.lib.model. """ + self.test_issue__repr__() + repo = pagure.lib.get_project(self.session, 'test') + issues = pagure.lib.search_issues(self.session, repo) + self.assertEqual(len(issues), 1) + + item = pagure.lib.model.TagColored( + tag='foo', + tag_color='DeepSkyBlue', + project_id=repo.id) + self.session.add(item) + self.session.commit() + + item = pagure.lib.model.TagIssueColored( + issue_uid=issues[0].uid, + tag_id=item.id + ) + self.session.add(item) + self.session.commit() + self.assertEqual( + str(item), + 'TagIssueColored(issue:1, tag:foo, project:test)' + ) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureLibModeltests) From 717597f7583a143eeb3d7da48b35118839537e1d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 5/9] Add the alembic migration allowing to migrate tags to the new model --- diff --git a/alembic/versions/208b0cd232ab_colored_tags.py b/alembic/versions/208b0cd232ab_colored_tags.py new file mode 100644 index 0000000..de1e3dd --- /dev/null +++ b/alembic/versions/208b0cd232ab_colored_tags.py @@ -0,0 +1,69 @@ +"""colored tags + + +Revision ID: 208b0cd232ab +Revises: 588eabcd394c +Create Date: 2016-12-27 11:52:53.355838 + +""" + +# revision identifiers, used by Alembic. +revision = '208b0cd232ab' +down_revision = '588eabcd394c' + +import collections +import datetime + +from alembic import op +import sqlalchemy as sa + + +try: + from pagure.lib import model +except ImportError: + import sys + sys.path.insert(0, '.') + from pagure.lib import model + + +def upgrade(): + """ Alter the DB schema for the changes related to colored tags. """ + engine = op.get_bind() + Session = sa.orm.scoped_session(sa.orm.sessionmaker()) + Session.configure(bind=engine) + session = Session() + + projects = collections.defaultdict(list) + for issue in session.query(model.Issue).all(): + for issuetag in issue.old_tags: + tag = issuetag.tag + # Add the tag to the project if it isn't already there + if tag not in projects[issue.project.id]: + tagobj = model.TagColored( + tag=tag, + project_id=issue.project.id) + session.add(tagobj) + session.flush() + projects[issue.project.id].append(tag) + else: + tagobj = session.query( + model.TagColored + ).filter( + model.TagColored.tag == tag + ).filter( + model.TagColored.project_id == issue.project.id + ).first() + + # Link the tag to the ticket as it was + tagissueobj = model.TagIssueColored( + tag_id=tagobj.id, + issue_uid=issue.uid, + date_created=tagobj.date_created, + ) + session.add(tagissueobj) + session.flush() + session.commit() + + +def downgrade(): + raise ValueError("The colored tags feature can not be un-done") From 40636d0b9bcca70f4ed9b20bcd087c80701fa36c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 6/9] Include the identifier in the string representation of the TagColored object --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 9ae2efe..ad01277 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1108,7 +1108,8 @@ class TagColored(BASE): ) def __repr__(self): - return 'TagColored(tag:%s, color:%s)' % (self.tag, self.tag_color) + return 'TagColored(id: %s, tag:%s, color:%s)' % ( + self.id, self.tag, self.tag_color) class TagIssueColored(BASE): From bee06846e07f1b9ba3e07fdb200bd282eab358e4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 7/9] Rework the logic in update_tags in pagure/ui/issues.py as suggested by @farhaan --- diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 56147ea..39a9cd4 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -443,40 +443,33 @@ def update_tags(repo, username=None, namespace=None): error = False if form.validate_on_submit(): - tag_names = flask.request.form.getlist('tag') - tag_colors = flask.request.form.getlist('tag_color') - - tags = [] - colors = [] - for t in range(len(tag_names)): - if tag_names[t] == "": - # Blank field, ignore - continue - tags.append(tag_names[t].strip()) - colors.append(tag_colors[t].strip()) + # Uniquify and order preserving + seen = set() + tags = [ + tag.strip() + for tag in flask.request.form.getlist('tag') + if tag.strip() and tag.strip() not in seen and not seen.add(tag.strip()) + ] + # Uniquify and order preserving + seen = set() + colors = [ + col.strip() + for col in flask.request.form.getlist('tag_color') + if col.strip() and col.strip() not in seen and not seen.add(col.strip()) + ] if len(tags) != len(colors): flask.flash( 'tags and tag colors are not of the same length', 'error') error = True - for tag in tags: - if tag.strip() and tags.count(tag) != 1: - flask.flash( - 'Tag %s is present %s times' % ( - tag, tags.count(tag) - ), - 'error') - error = True - break - if not error: - for cnt in range(len(tags)): + for idx, tag in enumerate(tags): try: pagure.lib.new_tag( SESSION, - tags[cnt].strip(), - colors[cnt], + tag, + colors[idx], repo.id) SESSION.commit() flask.flash('Tags updated') From 0c0101bfba20fe0a8ced9e84b12c1802ab9211b0 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 8/9] Ensure the provided color match the expected pattern We do not want to let user upload arbitrary strings that will then be displayed in the UI. --- diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 39a9cd4..2a12cb4 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -17,6 +17,7 @@ import datetime import flask import os +import re from collections import defaultdict from math import ceil @@ -458,6 +459,14 @@ def update_tags(repo, username=None, namespace=None): if col.strip() and col.strip() not in seen and not seen.add(col.strip()) ] + color_pattern = re.compile('^#\w{3,6}$') + for color in colors: + if not color_pattern.match(color): + flask.flash( + 'Color: %s does not match the expected pattern' % color, + 'error') + error = True + if len(tags) != len(colors): flask.flash( 'tags and tag colors are not of the same length', 'error') From 2fa745829df912c7303febfac21704e63c4318fc Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 05 2017 15:24:18 +0000 Subject: [PATCH 9/9] Add unit-tests for the update_tags endpoint --- diff --git a/tests/test_pagure_flask_ui_issues.py b/tests/test_pagure_flask_ui_issues.py index 6e79890..c71bd30 100644 --- a/tests/test_pagure_flask_ui_issues.py +++ b/tests/test_pagure_flask_ui_issues.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- """ - (c) 2015-2016 - Copyright Red Hat Inc + (c) 2015-2017 - Copyright Red Hat Inc Authors: Pierre-Yves Chibon @@ -1965,6 +1965,137 @@ class PagureFlaskIssuestests(tests.Modeltests): self.assertIn( '
Issues GIT URLs
', output.data) + @patch('pagure.lib.git.update_git') + @patch('pagure.lib.notify.send_email') + def test_update_tags(self, p_send_email, p_ugt): + """ Test the update_tags endpoint. """ + p_send_email.return_value = True + p_ugt.return_value = True + + # No Git repo + output = self.app.post('/foo/update/tags') + self.assertEqual(output.status_code, 404) + + user = tests.FakeUser() + with tests.user_set(pagure.APP, user): + output = self.app.post('/foo/update/tags') + self.assertEqual(output.status_code, 404) + + tests.create_projects(self.session) + tests.create_projects_git(self.path) + + output = self.app.post('/test/update/tags') + self.assertEqual(output.status_code, 403) + + # User not logged in + output = self.app.post('/test/update/tags') + self.assertEqual(output.status_code, 302) + + # Create issues to play with + repo = pagure.lib.get_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ticketfolder=None + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + # Before update, list tags + tags = pagure.lib.get_tags_of_project(self.session, repo) + self.assertEqual([tag.tag for tag in tags], []) + + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + # No CSRF + data = { + 'tag': 'red', + 'tag_color': '#ff0000' + } + output = self.app.post( + '/test/update/tags', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn( + '
    ' + '\n
', output.data) + + csrf_token = output.data.split( + 'name="csrf_token" type="hidden" value="')[1].split('">')[0] + + # Invalid color + data = { + 'tag': 'red', + 'tag_color': 'red', + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/tags', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn( + '\n ' + 'Color: red does not match the expected pattern', + output.data) + self.assertIn( + '
    ' + '\n
', output.data) + + # Inconsistent length color + data = { + 'tag': ['red', 'blue'], + 'tag_color': 'red', + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/tags', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn( + '\n ' + 'Color: red does not match the expected pattern', + output.data) + self.assertIn( + '\n tags and tag colors' + ' are not of the same length', output.data) + self.assertIn( + '
    ' + '\n
', output.data) + + # Valid query + data = { + 'tag': ['red', 'blue'], + 'tag_color': ['#ff0000', '#003cff'], + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/tags', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn( + 'blue', output.data) + self.assertIn( + '', + output.data) + self.assertIn( + 'red', output.data) + self.assertIn( + '', + output.data) + + # After update, list tags + tags = pagure.lib.get_tags_of_project(self.session, repo) + self.assertEqual([tag.tag for tag in tags], ['blue', 'red']) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureFlaskIssuestests)