-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 due date #3794
Issue due date #3794
Changes from 32 commits
406cd5a
859cd47
02415f7
bfef995
74c9e07
aeb3db9
211b2ed
82d7bf0
13a12c3
c1ada01
6663012
f4baf19
06d9649
3bf2227
60bbb23
192f31e
f766101
a1b79fe
e659699
bdec65a
38e1180
51d3e4f
5c93fe0
7292c30
6fb31d6
3ae5b7a
0b18607
2efba88
d8ba5ab
674d2ed
b8760ba
c88a59d
7c0e537
30a2c70
bf2d849
30dbda4
b9f193a
3c8216f
fb7463f
1c47d90
81c93d6
d02d09c
253d48f
c17c95b
d7f4798
f35fec0
1412580
fe68965
65e841d
311dc73
8d3fff2
3f570d4
a7a9529
d49dd2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,9 +47,10 @@ type Issue struct { | |
Ref string | ||
|
||
DeadlineUnix util.TimeStamp `xorm:"INDEX"` | ||
CreatedUnix util.TimeStamp `xorm:"INDEX created"` | ||
UpdatedUnix util.TimeStamp `xorm:"INDEX updated"` | ||
ClosedUnix util.TimeStamp `xorm:"INDEX"` | ||
|
||
CreatedUnix util.TimeStamp `xorm:"INDEX created"` | ||
UpdatedUnix util.TimeStamp `xorm:"INDEX updated"` | ||
ClosedUnix util.TimeStamp `xorm:"INDEX"` | ||
|
||
Attachments []*Attachment `xorm:"-"` | ||
Comments []*Comment `xorm:"-"` | ||
|
@@ -69,6 +70,11 @@ func init() { | |
issueTasksDonePat = regexp.MustCompile(issueTasksDoneRegexpStr) | ||
} | ||
|
||
// IsOverdue checks if the issue is overdue | ||
func (issue *Issue) IsOverdue() bool { | ||
return util.TimeStampNow() >= issue.DeadlineUnix | ||
} | ||
|
||
func (issue *Issue) loadRepo(e Engine) (err error) { | ||
if issue.Repo == nil { | ||
issue.Repo, err = getRepositoryByID(e, issue.RepoID) | ||
|
@@ -324,6 +330,9 @@ func (issue *Issue) APIFormat() *api.Issue { | |
apiIssue.PullRequest.Merged = issue.PullRequest.MergedUnix.AsTimePtr() | ||
} | ||
} | ||
if issue.DeadlineUnix != 0 { | ||
apiIssue.Deadline = issue.DeadlineUnix.AsTimePtr() | ||
} | ||
|
||
return apiIssue | ||
} | ||
|
@@ -1498,3 +1507,46 @@ func updateIssue(e Engine, issue *Issue) error { | |
func UpdateIssue(issue *Issue) error { | ||
return updateIssue(x, issue) | ||
} | ||
|
||
// UpdateIssueDeadline does what it says | ||
func UpdateIssueDeadline(issue *Issue, doer *User) (err error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can change this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But wouldn't that mean to get the actual deadline before calling the function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like IMO only issue deadline should be updated, not everything. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something like this?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I'd still need to check if the issue deadline hasn't changed meanwhile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe check if new deadline is different than old deadline? So we don't create comments nor update issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I haven't thought of that. |
||
|
||
// Check if the new date was added or modified | ||
var actualIssue Issue | ||
if _, err := x.ID(issue.ID).Get(&actualIssue); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed it to only get and update |
||
return fmt.Errorf("getActualIssue: %v", err) | ||
} | ||
|
||
sess := x.NewSession() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now, do we really need this session? What you want to achieve here? From xorm readme:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need that session to create the comment. |
||
defer sess.Close() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, fixed. |
||
|
||
if err = issue.loadRepo(sess); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed. |
||
return fmt.Errorf("loadRepo: %v", err) | ||
} | ||
|
||
// Make the comment | ||
if issue.DeadlineUnix == 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can move this if/else to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
if _, err = createDeadlineComment(sess, doer, issue.Repo, issue, actualIssue.DeadlineUnix, CommentTypeRemovedDeadline); err != nil { | ||
return fmt.Errorf("createRemovedDueDateComment: %v", err) | ||
} | ||
} else { | ||
// Check if the new date was added or modified | ||
// If the actual deadline is 0 => deadline added | ||
if actualIssue.DeadlineUnix == 0 { | ||
if _, err = createDeadlineComment(sess, doer, issue.Repo, issue, issue.DeadlineUnix, CommentTypeAddedDeadline); err != nil { | ||
return fmt.Errorf("createRemovedDueDateComment: %v", err) | ||
} | ||
} else { // Otherwise modified | ||
if _, err = createDeadlineComment(sess, doer, issue.Repo, issue, issue.DeadlineUnix, CommentTypeModifiedDeadline); err != nil { | ||
return fmt.Errorf("createRemovedDueDateComment: %v", err) | ||
} | ||
} | ||
} | ||
|
||
err = updateIssue(sess, issue) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return sess.Commit() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want transaction, you have to call
If not, this is not needed and you can just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll add that. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,12 @@ const ( | |
CommentTypeAddTimeManual | ||
// Cancel a stopwatch for time tracking | ||
CommentTypeCancelTracking | ||
// Added a due date | ||
CommentTypeAddedDeadline | ||
// Modified the due date | ||
CommentTypeModifiedDeadline | ||
// Removed a due date | ||
CommentTypeRemovedDeadline | ||
) | ||
|
||
// CommentTag defines comment tag type | ||
|
@@ -485,6 +491,25 @@ func createAssigneeComment(e *xorm.Session, doer *User, repo *Repository, issue | |
}) | ||
} | ||
|
||
func createDeadlineComment(e *xorm.Session, doer *User, repo *Repository, issue *Issue, dateUnix util.TimeStamp, deadlineCommentType CommentType) (*Comment, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It would be great if content is more specific. When deadline is modified it should show both dates. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but I rather won't add another row to the issue comment table as there are already too much. Someone said we should redo that some day, so I'll probably wait for that to do that... Or do you have an idea on how to add both dates in one field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant if we can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sure. I'll figure something out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
if deadlineCommentType != CommentTypeAddedDeadline && | ||
deadlineCommentType != CommentTypeModifiedDeadline && | ||
deadlineCommentType != CommentTypeRemovedDeadline { | ||
return &Comment{}, fmt.Errorf("wrong comment type: %d", deadlineCommentType) | ||
} | ||
|
||
// Make string from unix date | ||
date := dateUnix.Format("2006-01-02") | ||
|
||
return createComment(e, &CreateCommentOptions{ | ||
Type: deadlineCommentType, | ||
Doer: doer, | ||
Repo: repo, | ||
Issue: issue, | ||
Content: date, | ||
}) | ||
} | ||
|
||
func createChangeTitleComment(e *xorm.Session, doer *User, repo *Repository, issue *Issue, oldTitle, newTitle string) (*Comment, error) { | ||
return createComment(e, &CreateCommentOptions{ | ||
Type: CommentTypeChangeTitle, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -737,6 +737,20 @@ issues.add_time_sum_to_small = No time was entered. | |
issues.cancel_tracking = Cancel | ||
issues.cancel_tracking_history = `cancelled time tracking %s` | ||
issues.time_spent_total = Total Time Spent | ||
issues.due_date = Due date | ||
issues.invalid_due_date_format = "Due date format is invalid, must be 'yyyy-mm-dd'." | ||
issues.error_modifying_due_date = "An error occured while modifying the due date." | ||
issues.error_removing_due_date = "An error occured while remvoing the due date." | ||
issues.due_date_form = "Due date, format yyyy-mm-dd" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that
could be confusing for users from non-english countries (like Germany) because modern browsers use for the UI the local date format (in Germany: dd.mm.yyyy) and as value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but that value is used as a placeholder for the input. If the browser supports Some screenshots with an empty input: The format doesnt really seem to be an issue here. Also, currently the date is not localized anywhere, so if we'd start doing that here we should do it everywhere which is totally out of scope for this pr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the placeholder isn't shown at all it could be removed to reduce translation effort. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is shown when the browser doesn't support the |
||
issues.due_date_form_add = "Add due date" | ||
issues.due_date_form_update = "Update due date" | ||
issues.due_date_form_remove = "Remove due date" | ||
issues.due_date_not_writer = "You need to have at least write access to this repository in order to update the due date for this issue." | ||
issues.due_date_not_set = "No due date set." | ||
issues.due_date_added = "added the due date %s %s" | ||
issues.due_date_modified = "modified the due date to %s %s" | ||
issues.due_date_remove = "removed the due date %s %s" | ||
issues.due_date_overdue = "Overdue" | ||
|
||
pulls.desc = Enable merge requests and code reviews. | ||
pulls.new = New Pull Request | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1544,6 +1544,9 @@ | |
margin-top: -5px; | ||
margin-right: 5px; | ||
} | ||
.overdue{ | ||
color: red; | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct comment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? Do you want me to write something like
UpdateIssueDeadline updates an issue deadline
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's better. And does it really update only deadline? I think it updates all given fields right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also creates comments. I am not sure if it is good. Maybe better to add comments directly in udate/remove deadline function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it updates all fields to its old value + issueDeadline to the new value. Although I think xorm does some magic here and only updates the fields which did not change.
This is the function which updates/removes the deadline...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the only diff between this function and
updateIssue
is that this func creates comments. Function doesn't know what are the fields ofissue
, who and why set it to given values, only caller knows it (function doesn't know context about when it is called, I can call it from anywhere). If I look at implementation, more meaningful explanation is "UpdateIssueDeadline creates deadline comment and updates issue".I don't think Xorm is so magical. As I know, it can ignore default values, but doesn't know what is updated. But you can check it by turning on sql logging so we can be sure about that.
Sry, I should be more concrete about "update/remove" functions. I meant
UpdateDeadline
andRemoveDeadline
. But as there is notxorm.Session
it can not be called from there directly.Edit: OK, you updated it in meanwhile so this comment is not 100% relevant now :)