Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timestamp in locked message for updates #832

Merged
merged 8 commits into from May 25, 2016

Conversation

trishnaguha
Copy link
Contributor

Fixes #831

@sayanchowdhury
Copy link

Looks good to me 👍

def date_locked(self):
""" Return the date when the update has been locked """
if self.locked:
return datetime.utcnow()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this always return utcnow()?
So today, it won't it show in the template as:
This update is currently locked since <small>2016-05-03 07:59:51 (UTC)</small> and cannot be modified.
In one hour:
This update is currently locked since <small>2016-05-03 08:59:51 (UTC)</small> and cannot be modified.
And tomorrow:
This update is currently locked since <small>2016-05-04 07:59:51 (UTC)</small> and cannot be modified.
?

@trishnaguha
Copy link
Contributor Author

trishnaguha commented May 6, 2016

@pypingou Yes that's right!
What if we can do something like this https://paste.fedoraproject.org/363289/ ?
I need some hint about its scope if this is good to go :).

# Assigns the current date when update is locked
locked_date = False
if self.locked:
    date_now = datetime.utcnow()
    locked_date = True
def date_locked(self):
    if self.locked_date == True:
        return self.date_now

@pypingou
Copy link
Member

pypingou commented May 6, 2016

Why not just saving the date in the DB itself as NULL by default and filling it at the time the update is updated to be marked as 'locked'? Then the date could be reset to NULL when the update is updated to be marked as 'unlocked' again.

@trishnaguha
Copy link
Contributor Author

@lmacken Can you please take a look at this one? :)
As @pypingou suggested on irc that you may have a better idea to implement this.
Can you suggest your idea so that I could modify the patch? :)

@lmacken
Copy link
Contributor

lmacken commented May 12, 2016

Yeah, I think we should probably add a new timestamp for the date locked to the Update model:

https://github.com/fedora-infra/bodhi/blob/develop/bodhi/models/models.py#L633-L639

Then in the masher when we lock/unlock updates, we should modify these timestamps.

https://github.com/fedora-infra/bodhi/blob/develop/bodhi/consumers/masher.py

@trishnaguha
Copy link
Contributor Author

@lmacken I have modified the PR.
Do we need to write test for the same?
/cc @sayanchowdhury @pypingou

@@ -506,7 +506,7 @@
<div class="alert alert-info" role="alert">
<span class="glyphicon glyphicon-lock" aria-hidden="true"></span>
<span class="sr-only">Locked:</span>
This update is currently locked and cannot be modified.
This update is currently locked since <small>${str(update.date_locked).split('.')[0]} (UTC)</small> and cannot be modified.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using str() here, I would suggest using .strftime() https://docs.python.org/2/library/time.html#time.strftime

@@ -378,6 +379,9 @@ def work(self):
self.check_all_karma_thresholds()
self.obsolete_older_updates()

# Update datetime for locking on update
self.locked_date_for_update()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will set the locked_date for each update after the push is done. Don't we want to set this when it is initially locked, at the beginning of the work method?

@lmacken
Copy link
Contributor

lmacken commented May 25, 2016

Sweet, LGTM! 👍

@lmacken lmacken merged commit 3d020dc into fedora-infra:develop May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants