Skip to content

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

Merged
jonasfranz merged 7 commits into
go-gitea:masterfrom
rvillablanca:fix-5566
Jan 1, 2019
Merged

Issue is not overdue when it is on the same date #5566#5568
jonasfranz merged 7 commits into
go-gitea:masterfrom
rvillablanca:fix-5566

Conversation

@rvillablanca

Copy link
Copy Markdown
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

codecov-io commented Dec 19, 2018

Copy link
Copy Markdown

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
Comment thread models/issue.go Outdated
Comment thread models/issue_milestone.go Outdated
@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
Comment thread models/issue.go Outdated

Copy link
Copy Markdown
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.

@lunny lunny Dec 23, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
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

@rvillablanca rvillablanca Dec 23, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread routers/api/v1/repo/issue.go
@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

lunny commented Dec 28, 2018

Copy link
Copy Markdown
Member

@kolaente need your approval.

@kolaente kolaente left a comment

Copy link
Copy Markdown
Member

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