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

Issue is not overdue when it is on the same date #5566 #5568

Merged
merged 7 commits into from
Jan 1, 2019

Conversation

rvillablanca
Copy link
Contributor

This PR fix #5566 so now a due date that is today won't be considered overdue, this applies to milestones and issues.

@codecov-io
Copy link

codecov-io commented Dec 19, 2018

Codecov Report

Merging #5568 into master will decrease coverage by <.01%.
The diff coverage is 10%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5568      +/-   ##
=========================================
- Coverage   37.81%   37.8%   -0.01%     
=========================================
  Files         322     322              
  Lines       47458   47463       +5     
=========================================
- Hits        17946   17944       -2     
- Misses      26926   26933       +7     
  Partials     2586    2586
Impacted Files Coverage Δ
routers/repo/milestone.go 0% <0%> (ø) ⬆️
routers/api/v1/repo/issue.go 30.02% <12.5%> (-0.27%) ⬇️
models/repo_list.go 63.29% <0%> (-1.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63bd1b9...d3abbaa. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 19, 2018
@techknowlogick techknowlogick added this to the 1.7.0 milestone Dec 19, 2018
models/issue.go Outdated Show resolved Hide resolved
models/issue_milestone.go Outdated Show resolved Hide resolved
@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 22, 2018
models/issue.go Outdated
@@ -83,7 +84,8 @@ func (issue *Issue) loadTotalTimes(e Engine) (err error) {

// IsOverdue checks if the issue is overdue
func (issue *Issue) IsOverdue() bool {
return util.TimeStampNow() >= issue.DeadlineUnix
deadline := issue.DeadlineUnix.AsTime().UTC().Round(24 * time.Hour)
return time.Now().UTC().After(deadline)
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather change the behaviour when saving a due date to save it on 23:59:59 instead of 00:00 rather than changing this when showing the due date.

Copy link
Member

@lunny lunny Dec 23, 2018

Choose a reason for hiding this comment

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

return util.TimeStampNow() >= issue.DeadlineUnix + util.TimeStamp(time.Hour * 24 / time.Second)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with kolaente

Copy link
Contributor Author

@rvillablanca rvillablanca Dec 23, 2018

Choose a reason for hiding this comment

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

Ok thinking about the solution It would be like this

if form.Deadline != nil && !form.Deadline.IsZero() {
	t := form.Deadline
	deadLine := time.Date(t.Year(), t.Month(), t.Day(), 23, 59, 59, 0, t.Location())
	form.Deadline = &deadLine
	deadlineUnix = util.TimeStamp(form.Deadline.Unix())
}

This is an example for the issue deadline case. What do you think @lunny, @kolaente, @Bwko, @adelowo ? Maybe this code could be simplified.

Copy link
Member

Choose a reason for hiding this comment

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

I'd only set it to 23:59:59 if it is set to 00:00:00. Maybe someone wants to use it in some kind of frontend with exact deadlines. Or maybe don't modify it at all on the backend and only in gitea's frontend?

Copy link
Member

Choose a reason for hiding this comment

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

@rvillablanca I don't against @kolaente 's idea, but that you have to consider old data migration.

Copy link
Member

Choose a reason for hiding this comment

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

@kolaente I don't think the frontend currently supports exact deadlines.

Copy link
Member

Choose a reason for hiding this comment

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

@adelowo Ours doesn't, but the api does.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should support exact deadlines (with time)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the behaviour of this, now the time part of a due date of issues and milestones is setted to 23:59:59 at the save/edit moment, this is according to @kolaente's suggestion.

With respect to exact deadlines I think that could be possible but at the moment due dates are being treated like a date and not like timestamp (this at least in the handlers, I know the api supports timestamp), so I believe this could be done in another PR as a new feature.

@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 28, 2018
@lunny
Copy link
Member

lunny commented Dec 28, 2018

@kolaente need your approval.

Copy link
Member

@kolaente kolaente left a comment

Choose a reason for hiding this comment

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

Looks good! I think the CI failed though...

@jonasfranz jonasfranz merged commit 4c52858 into go-gitea:master Jan 1, 2019
@rvillablanca rvillablanca deleted the fix-5566 branch January 2, 2019 11:24
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues and Milestones due today are marked as overdue
10 participants