From 23b4d2b81c03a76e95f14f685aaa5d436ff661e2 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Nov 30 2016 11:41:57 +0000 Subject: [PATCH 1/5] Issue 1549 - Add "updated_on" to Issues and make it queryable Added updated_on to track the time/date of when the issue was last updated. Updated the Issues/Issue html pages to utilize the new parameter. Added a new search argument for issues called "since" that accepts either a timestamp or a datetime string, and returns all the issues from that time and forward: /api/0/MY_PROJECT/issues?since=1479261135 or /api/0/MY_PROJECT/issues?since=2016-11-15 https://pagure.io/pagure/issue/1549 --- diff --git a/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py b/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py new file mode 100644 index 0000000..8ad60a1 --- /dev/null +++ b/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py @@ -0,0 +1,39 @@ +"""add last_update to issues and pull-requests + +Revision ID: 114d3a68c1fd +Revises: 5083efccac7 +Create Date: 2016-11-15 11:02:30.652540 + +""" + +# revision identifiers, used by Alembic. +revision = '114d3a68c1fd' +down_revision = '5083efccac7' + +from alembic import op +import sqlalchemy as sa +import datetime + + +def upgrade(): + ''' Add the column last_updated to the table issues/pull-requests + ''' + op.add_column( + 'issues', + sa.Column('last_updated', sa.DateTime, nullable=True, + default=sa.datetime.datetime.utcnow, + onupdate=datetime.datetime.utcnow) + ) + op.add_column( + 'pull_requests', + sa.Column('last_updated', sa.DateTime, nullable=True, + default=sa.datetime.datetime.utcnow, + onupdate=datetime.datetime.utcnow) + ) + + +def downgrade(): + ''' Drop the column last_update from the table issues/pull-requests + ''' + op.drop_column('issues', 'last_updated') + op.drop_column('pull_requests', 'last_updated') diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 37a29f7..682998e 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -78,6 +78,8 @@ class APIERROR(enum.Enum): ENOCOMMENT = 'Comment not found' ENEWPROJECTDISABLED = 'Creating project have been disabled for this '\ 'instance' + ETIMESTAMP = 'Invalid timestamp format' + EDATETIME = 'Invalid datetime format' def check_api_acls(acls, optional=False): diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 376a001..2f71447 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -9,6 +9,7 @@ """ import flask +import datetime from sqlalchemy.exc import SQLAlchemyError @@ -273,6 +274,7 @@ def api_view_issues(repo, username=None, namespace=None): tags = [tag.strip() for tag in tags if tag.strip()] assignee = flask.request.args.get('assignee', None) author = flask.request.args.get('author', None) + last_updated = flask.request.args.get('since', None) # Hide private tickets private = False @@ -301,9 +303,29 @@ def api_view_issues(repo, username=None, namespace=None): params.update({'status': status}) issues = pagure.lib.search_issues(**params) else: + lastupdated = None + if last_updated: + # Validate and convert the time + if last_updated.isdigit(): + # We assume its a timestamp, so convert it to datetime + try: + lastupdated = \ + datetime.datetime.fromtimestamp(int(last_updated)) + except: + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.ETIMESTAMP) + else: + # We assume datetime format, so validate it + try: + datetime.datetime.strptime(last_updated, '%Y-%m-%d') + except: + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.EDATETIME) + lastupdated = last_updated + issues = pagure.lib.search_issues( SESSION, repo, status='Open', tags=tags, assignee=assignee, - author=author, private=private) + author=author, private=private, last_updated=lastupdated) jsonout = flask.jsonify({ 'total_issues': len(issues), @@ -313,6 +335,7 @@ def api_view_issues(repo, username=None, namespace=None): 'tags': tags, 'assignee': assignee, 'author': author, + 'since': last_updated } }) return jsonout diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 3995f2c..055af66 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -386,6 +386,7 @@ def add_issue_assignee(session, issue, assignee, user, ticketfolder, if assignee is None and issue.assignee is not None: issue.assignee_id = None + issue.last_updated = datetime.datetime.utcnow() session.add(issue) session.commit() pagure.lib.git.update_git( @@ -439,6 +440,7 @@ def add_issue_assignee(session, issue, assignee, user, ticketfolder, ), redis=REDIS, ) + issue.last_updated = datetime.datetime.utcnow() # Send notification for the event-source server if REDIS: @@ -456,6 +458,7 @@ def add_pull_request_assignee( if assignee is None and request.assignee is not None: request.assignee_id = None + request.last_updated = datetime.datetime.utcnow() session.add(request) session.commit() pagure.lib.git.update_git( @@ -482,6 +485,7 @@ def add_pull_request_assignee( if request.assignee_id != assignee_obj.id: request.assignee_id = assignee_obj.id + request.last_updated = datetime.datetime.utcnow() session.add(request) session.flush() pagure.lib.git.update_git( @@ -882,6 +886,8 @@ def add_pull_request_comment(session, request, commit, tree_id, filename, # Make sure we won't have SQLAlchemy error before we continue session.flush() + request.last_updated = datetime.datetime.utcnow() + pagure.lib.git.update_git( request, repo=request.project, repofolder=requestfolder) @@ -938,6 +944,7 @@ def edit_comment(session, parent, comment, user, comment.comment = updated_comment comment.edited_on = datetime.datetime.utcnow() comment.editor = user_obj + parent.last_updated = comment.edited_on session.add(comment) # Make sure we won't have SQLAlchemy error before we continue @@ -1206,6 +1213,7 @@ def new_issue(session, repo, title, content, user, ticketfolder, issue.status = status if close_status is not None: issue.close_status = close_status + issue.last_updated = datetime.datetime.utcnow() session.add(issue) # Make sure we won't have SQLAlchemy error before we create the issue @@ -1289,6 +1297,8 @@ def new_pull_request(session, branch_from, user_id=user_obj.id, status=status, ) + request.last_updated = datetime.datetime.utcnow() + session.add(request) # Make sure we won't have SQLAlchemy error before we create the request session.flush() @@ -1364,6 +1374,7 @@ def edit_issue(session, issue, ticketfolder, user, if milestone != issue.milestone: issue.milestone = milestone edit.append('milestone') + issue.last_updated = datetime.datetime.utcnow() pagure.lib.git.update_git( issue, repo=issue.project, repofolder=ticketfolder) @@ -1728,11 +1739,12 @@ def get_project(session, name, user=None, namespace=None): return query.first() + def search_issues( session, repo, issueid=None, issueuid=None, status=None, closed=False, tags=None, assignee=None, author=None, private=None, priority=None, milestones=None, count=False, offset=None, - limit=None, search_pattern=None, custom_search=None): + limit=None, search_pattern=None, custom_search=None, last_updated=None): ''' Retrieve one or more issues associated to a project with the given criterias. @@ -1784,6 +1796,8 @@ def search_issues( :kwarg custom_search: a dictionary of key/values to be used when searching issues with a custom key constraint :type custom_search: dict or None + :kwarg last_updated: datetime's date format (e.g. 2016-11-15) + :type last_updated: str or None :return: A single Issue object if issueid is specified, a list of Project objects otherwise. @@ -1796,6 +1810,11 @@ def search_issues( model.Issue.project_id == repo.id ) + if last_updated: + query = query.filter( + model.Issue.last_updated >= last_updated + ) + if issueid is not None: query = query.filter( model.Issue.id == issueid @@ -2025,7 +2044,7 @@ def get_tag(session, tag): def search_pull_requests( session, requestid=None, project_id=None, project_id_from=None, status=None, author=None, assignee=None, count=False, - offset=None, limit=None): + offset=None, limit=None, last_updated=None): ''' Retrieve the specified issue ''' @@ -2040,6 +2059,11 @@ def search_pull_requests( model.PullRequest.id == requestid ) + if last_updated: + query = query.filter( + model.PullRequest.last_updated >= last_updated + ) + if project_id: query = query.filter( model.PullRequest.project_id == project_id diff --git a/pagure/lib/model.py b/pagure/lib/model.py index ab6e6fe..be14733 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -663,10 +663,11 @@ class Issue(BASE): priority = sa.Column(sa.Integer, nullable=True, default=None) milestone = sa.Column(sa.String(255), nullable=True, default=None) close_status = sa.Column(sa.Text, nullable=True) - date_created = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow) - + last_updated = sa.Column(sa.DateTime, nullable=True, + default=datetime.datetime.utcnow, + onupdate=datetime.datetime.utcnow) closed_at = sa.Column(sa.DateTime, nullable=True) project = relation( @@ -749,6 +750,7 @@ class Issue(BASE): 'status': self.status, 'close_status': self.close_status, 'date_created': self.date_created.strftime('%s'), + 'last_updated': self.last_updated.strftime('%s'), 'closed_at': self.closed_at.strftime( '%s') if self.closed_at else None, 'user': self.user.to_json(public=public), @@ -1118,6 +1120,10 @@ class PullRequest(BASE): default=sa.func.now(), onupdate=sa.func.now()) + last_updated = sa.Column(sa.DateTime, nullable=True, + default=datetime.datetime.utcnow, + onupdate=datetime.datetime.utcnow) + __table_args__ = ( sa.CheckConstraint( 'NOT(project_id_from IS NULL AND remote_git IS NULL)', @@ -1223,6 +1229,7 @@ class PullRequest(BASE): 'remote_git': self.remote_git, 'date_created': self.date_created.strftime('%s'), 'updated_on': self.updated_on.strftime('%s'), + 'last_updated': self.last_updated.strftime('%s'), 'closed_at': self.closed_at.strftime( '%s') if self.closed_at else None, 'user': self.user.to_json(public=public), diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html index 7eef6ca..46bf5fb 100644 --- a/pagure/templates/issue.html +++ b/pagure/templates/issue.html @@ -36,6 +36,14 @@ issue.date_created.strftime('%b %d %Y %H:%M:%S') }}">{{ issue.date_created | humanize}} by {{ issue.user.user }} + {% if issue.last_updated %} +
+ Modified {{ issue.last_updated | humanize}} by {{ issue.user.user }} +
+ {% endif %} +
{{ show_initial_comment(issue, username, repo,issueid, form) }} diff --git a/pagure/templates/issues.html b/pagure/templates/issues.html index 2dde4a9..66b73ad 100644 --- a/pagure/templates/issues.html +++ b/pagure/templates/issues.html @@ -153,6 +153,8 @@ Opened {% if status and status|lower != 'open' %} Closed + {% else %} + Modified {% endif %} Priority ( + {{ + issue.last_updated | humanize}} + {% endif %} + Modified {{ pull_request.last_updated |humanize }} + by {{ pull_request.user.default_email | avatar(16) | safe + }} {{ pull_request.user.user }} + From {{ diff --git a/pagure/templates/requests.html b/pagure/templates/requests.html index fb96fc0..a8f0df1 100644 --- a/pagure/templates/requests.html +++ b/pagure/templates/requests.html @@ -85,6 +85,8 @@ Opened {% if status|lower != 'open' %} Closed + {% else %} + Modified {% endif %} Reporter({{ request.date_created | humanize}} + + {{ + request.last_updated | humanize}} + {% if status|lower not in ['open', 'true'] %} Date: Nov 30 2016 11:41:57 +0000 Subject: [PATCH 2/5] Adjust the alembic migration script to fill last_updated on existing rows This allows having a value for all the existing tickets and pull-requests when adding the last_updated column to the database. --- diff --git a/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py b/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py index 8ad60a1..89cf9a9 100644 --- a/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py +++ b/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py @@ -21,15 +21,35 @@ def upgrade(): op.add_column( 'issues', sa.Column('last_updated', sa.DateTime, nullable=True, - default=sa.datetime.datetime.utcnow, + default=datetime.datetime.utcnow, onupdate=datetime.datetime.utcnow) ) + op.execute(''' +UPDATE "issues" SET last_updated=o_date +FROM ( + SELECT issue_uid, GREATEST(date_created, edited_on) AS o_date + FROM issue_comments + ORDER BY o_date DESC +) AS subq +WHERE "issues".uid = issue_uid;''') + op.execute('''UPDATE "issues" SET last_updated=date_created ''' + '''WHERE last_updated IS NULL;''') op.add_column( 'pull_requests', sa.Column('last_updated', sa.DateTime, nullable=True, - default=sa.datetime.datetime.utcnow, + default=datetime.datetime.utcnow, onupdate=datetime.datetime.utcnow) ) + op.execute(''' +UPDATE "pull_requests" SET last_updated=o_date +FROM ( + SELECT pull_request_uid, GREATEST(date_created, edited_on) AS o_date + FROM pull_request_comments + ORDER BY o_date DESC +) AS subq +WHERE "pull_requests".uid = pull_request_uid;''') + op.execute('''UPDATE "pull_requests" SET last_updated=date_created ''' + '''WHERE last_updated IS NULL;''') def downgrade(): From 3bf0cd07831c14b0db78467915f85d3c3087b9e8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 30 2016 11:41:57 +0000 Subject: [PATCH 3/5] Make the last_updated fields in tickets/PRs not NULL at the DB level --- diff --git a/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py b/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py index 89cf9a9..051ecf1 100644 --- a/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py +++ b/alembic/versions/114d3a68c1fd_add_updated_on_column_to_issues.py @@ -24,6 +24,7 @@ def upgrade(): default=datetime.datetime.utcnow, onupdate=datetime.datetime.utcnow) ) + # Update all the tickets having comments op.execute(''' UPDATE "issues" SET last_updated=o_date FROM ( @@ -32,14 +33,21 @@ FROM ( ORDER BY o_date DESC ) AS subq WHERE "issues".uid = issue_uid;''') + # Update all the tickets without comments op.execute('''UPDATE "issues" SET last_updated=date_created ''' '''WHERE last_updated IS NULL;''') + # Require `last_updated` no NULL at the DB level + op.alter_column( + 'issues', 'last_updated', + nullable=False, existing_nullable=True) + op.add_column( 'pull_requests', sa.Column('last_updated', sa.DateTime, nullable=True, default=datetime.datetime.utcnow, onupdate=datetime.datetime.utcnow) ) + # Update all the PRs having comments op.execute(''' UPDATE "pull_requests" SET last_updated=o_date FROM ( @@ -48,8 +56,13 @@ FROM ( ORDER BY o_date DESC ) AS subq WHERE "pull_requests".uid = pull_request_uid;''') + # Update all the PRs without comments op.execute('''UPDATE "pull_requests" SET last_updated=date_created ''' '''WHERE last_updated IS NULL;''') + # Require `last_updated` no NULL at the DB level + op.alter_column( + 'pull_requests', 'last_updated', + nullable=False, existing_nullable=True) def downgrade(): diff --git a/pagure/lib/model.py b/pagure/lib/model.py index be14733..db1abd2 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -665,7 +665,7 @@ class Issue(BASE): close_status = sa.Column(sa.Text, nullable=True) date_created = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow) - last_updated = sa.Column(sa.DateTime, nullable=True, + last_updated = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow, onupdate=datetime.datetime.utcnow) closed_at = sa.Column(sa.DateTime, nullable=True) @@ -1120,7 +1120,7 @@ class PullRequest(BASE): default=sa.func.now(), onupdate=sa.func.now()) - last_updated = sa.Column(sa.DateTime, nullable=True, + last_updated = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow, onupdate=datetime.datetime.utcnow) From 4bf54f9ea41ca5d0c613ebd04357ea3c0316fc7e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 30 2016 11:41:57 +0000 Subject: [PATCH 4/5] Rename the filtering argument last_updated to updated_after This rename gives a better idea of what the filtering will do. --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 055af66..16ece56 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -1744,7 +1744,8 @@ def search_issues( session, repo, issueid=None, issueuid=None, status=None, closed=False, tags=None, assignee=None, author=None, private=None, priority=None, milestones=None, count=False, offset=None, - limit=None, search_pattern=None, custom_search=None, last_updated=None): + limit=None, search_pattern=None, custom_search=None, + updated_after=None): ''' Retrieve one or more issues associated to a project with the given criterias. @@ -1796,8 +1797,9 @@ def search_issues( :kwarg custom_search: a dictionary of key/values to be used when searching issues with a custom key constraint :type custom_search: dict or None - :kwarg last_updated: datetime's date format (e.g. 2016-11-15) - :type last_updated: str or None + :kwarg updated_after: datetime's date format (e.g. 2016-11-15) used to + filter issues updated after that date + :type updated_after: str or None :return: A single Issue object if issueid is specified, a list of Project objects otherwise. @@ -1810,9 +1812,9 @@ def search_issues( model.Issue.project_id == repo.id ) - if last_updated: + if updated_after: query = query.filter( - model.Issue.last_updated >= last_updated + model.Issue.last_updated >= updated_after ) if issueid is not None: @@ -2044,7 +2046,7 @@ def get_tag(session, tag): def search_pull_requests( session, requestid=None, project_id=None, project_id_from=None, status=None, author=None, assignee=None, count=False, - offset=None, limit=None, last_updated=None): + offset=None, limit=None, updated_after=None): ''' Retrieve the specified issue ''' @@ -2059,9 +2061,9 @@ def search_pull_requests( model.PullRequest.id == requestid ) - if last_updated: + if updated_after: query = query.filter( - model.PullRequest.last_updated >= last_updated + model.PullRequest.last_updated >= updated_after ) if project_id: From 1c7de5041c1eeca9b55f6496c026a723397a260c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 30 2016 11:41:57 +0000 Subject: [PATCH 5/5] Rename the variables as expected and raise a 400 error when needed This fixes a couple of issues found out in the review by @jcline. We were raising 404 instead of 400, so this is fixed. We updated the filtering argument from last_updated to updated_after so we took that opportunity to rename some of the other variables while at it. --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 2f71447..7462ea0 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -274,7 +274,7 @@ def api_view_issues(repo, username=None, namespace=None): tags = [tag.strip() for tag in tags if tag.strip()] assignee = flask.request.args.get('assignee', None) author = flask.request.args.get('author', None) - last_updated = flask.request.args.get('since', None) + since = flask.request.args.get('since', None) # Hide private tickets private = False @@ -303,29 +303,29 @@ def api_view_issues(repo, username=None, namespace=None): params.update({'status': status}) issues = pagure.lib.search_issues(**params) else: - lastupdated = None - if last_updated: + updated_after = None + if since: # Validate and convert the time - if last_updated.isdigit(): + if since.isdigit(): # We assume its a timestamp, so convert it to datetime try: - lastupdated = \ - datetime.datetime.fromtimestamp(int(last_updated)) - except: + updated_after = \ + datetime.datetime.fromtimestamp(int(since)) + except ValueError: raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETIMESTAMP) + 400, error_code=APIERROR.ETIMESTAMP) else: # We assume datetime format, so validate it try: - datetime.datetime.strptime(last_updated, '%Y-%m-%d') - except: + updated_after= datetime.datetime.strptime( + since, '%Y-%m-%d') + except ValueError: raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EDATETIME) - lastupdated = last_updated + 400, error_code=APIERROR.EDATETIME) issues = pagure.lib.search_issues( SESSION, repo, status='Open', tags=tags, assignee=assignee, - author=author, private=private, last_updated=lastupdated) + author=author, private=private, updated_after=updated_after) jsonout = flask.jsonify({ 'total_issues': len(issues), @@ -335,7 +335,7 @@ def api_view_issues(repo, username=None, namespace=None): 'tags': tags, 'assignee': assignee, 'author': author, - 'since': last_updated + 'since': since } }) return jsonout