Everywhere else in the code we're relying on datetime.datetime.utcnow so I would prefer that we stay consistent. (cf the change in the model.py)

updated_on has a rather strict meaning for me: it was updated on that day, so would you consider renaming this variable to updated_since?

I would prefer that we do not add a new dependency if we can avoid it and it sounds to me that we could do without here.

I am not 100% sure I like doing the validation in here vs keeping it in the API controller.

Also, did you consider simply relying on arrow.get() to handle the parsing?

cf my comment above about what we're using so far (and the line just above for date_created :)

A few comments but this is looking quite good already :)

One question though: would you be willing to add some unit-tests for this? Otherwise I can take care of it.

Jenkins is right, we'll also need to adjust the current unit-tests :)

Everywhere that uses DateTime uses datetime.datetime.utcnow() and vice versa. They go hand in hand. So I'm not sure what you are asking me to do.

I used "updated_on" because that is what is used for Users and Pull-Requests. I was trying to stay consistent with the other resources. I'm happy to change it though

The default and onupdate fields are using sa.func.now() while we could use datetime.datetime.utcnow

enum is used in other files: api/init.py In order to return an error I have to use a class like this to utilize the existing API for returning http errors.

I don't know "arrow". I'm not sure how/where else to do this

Sorry I don't follow, can you you explain?

ah, I see, for Users an PRs this field is meant to be updated when the objects themselves are.

For User the field is when they update their settings for PR, it's for when the PR is updated (new commits or so).

I wonder if it wouldn't make sense to have an last_update field or something similar.

What do you think?

Geez, you're right, sorry about that :)

in api/issues.py where this method is called?
Then we can just have updated_on be a datetime object directly

I was speaking about default and onupdate

I think having a "last modified" timestamp, for at least for pull-requests would be very useful. Not sure about Users (I don't know what that really is yet) :-)

Right, gotcha!

For Users they already have updated_on, last_modified for Issue and PR would be reflecting when a comment is added, so maybe last_comment_on would reflect more what we want it to be.

Okay, to summarise this topic: I'm going to change "update_on" to "last_update". I will also look into doing the same thing for Pull-Requests and Users.

I will also be applying your requests from other comments, then I will send a new "complete" patch out.

I don't think it's needed for Users, they already have updated_on which is doing just that :)

I am also seeing failures and errors when running runtets.py

My patch:
FAILED (errors=11, failures=11)

Master Branch:
FAILED (errors=11)

I do see this error below, can I safely ignore this?

[mareynol@mark pagure]$ ./runtests.sh 
BUILD_ID: None
......./home/mareynol/source/pagure/pagure/lib/notify.py:49: UserWarning: No module named fedmsg
  warnings.warn(str(err))

/home/mareynol/source/pagure/pagure/lib/notify.py:49: UserWarning: No module named fedmsg
warnings.warn(str(err))

This is safe to ignore yes

But on master, I see:

Ran 298 tests in 147.265s
OK (SKIP=2)

Fixed the tests. Attaching new patch to issue #1549

Looks like this needs a small update now that we're using a datetime

These should be the function, not be called, ie: without the () at the end.

same as above

Looks like we have an inconsistency here, I should fix that

This seems unrelated to that change-set/PR, were you fixing an issue you encountered?

We will need some unit-tests to check the behavior of the API as well :)

That's one of the things I have in mind to do tomorrow, unless you beat me to it :)

Right, nice catch

Oh yeah. So to test pull_requests in my dev environment I used a git repo not on pagure. The fork code did not like what it saw and spit out index exceptions because first_commit.parents was empty.

Should I remove this "fix" and open a new issue?

Oh yeah. So to test pull_requests in my dev environment I used a git repo not on pagure. The fork code did not like what it saw and spit out index exceptions because first_commit.parents was empty.

Should I remove this "fix" and open a new issue?

The best would be to make it its own commit so the commit message can explain
the change

I have had to tweak a little the alembic upgrade script to make it work:

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..3ff081a 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,23 @@ 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=date_created '''
+               '''WHERE status = 'Open';''')
+    op.execute('''UPDATE "issues" SET last_updated=closed_at '''
+               '''WHERE status != 'Open';''')
     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=date_created '''
+               '''WHERE status = 'Open';''')
+    op.execute('''UPDATE "pull_requests" SET last_updated=closed_at '''
+               '''WHERE status != 'Open';''')
 def downgrade():

Still testing it, will push the commit fixing the script later :)

Which makes me wonder: why do we allow last_updated to be null in the database?

I can take care of this if you want, but wanted to ask in case I miss something obvious :)

Which makes me wonder: why do we allow last_updated to be null in the database?
I can take care of this if you want, but wanted to ask in case I miss something obvious :)

Yeah, you're right. I think originally I was thinking that last_updated would not apply(or be NULL) for when the issue/PR was created. But it does get set at that time, so it would not be NULL. So yes, please change this.

I have a better update query for the alembic migration script and the NULL status fix, pushing

ready to go :+1:

datetime.datetime.fromtimestamp raises a ValueError if there's a problem, so it would be better to catch that explicitly here.

Since there's a check to make sure last_updated is castable to an int above, the only error I know of is if the integer is out of the platform's time_t range. The error message 'Invalid timestamp format' is a little misleading, given that.

Finally, I think maybe HTTP 400 is more appropriate.

Same as above comment

Maybe updated_after would be a better name? It's not clear until I look at the query itself that that is the purpose of this argument.

This also would be better as updated_after, I think.

Although it doesn't really hurt, do we actually need this since onupdate is set to this function for the last_updated column?

The only thing I'd really like to see changed is using an HTTP 400 rather than a 404 if the query parameter is bad. Everything else is just a suggestion.

Nice catch, this is wrong, probably a copy and paste error on my part. 400 would be the appropriate code.

Actually that line is needed. I thought the same thing about onupdate, but yet it did not work in this context - so I had to explicitly set it.

I have updated with @jcline 's suggestions, I'm rebasing so we're ready to merge :)

:thumbsup: Looks good

Thanks!