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

Merged
merged 54 commits into from May 1, 2018

Conversation

7 participants
@kolaente
Copy link
Member

kolaente commented Apr 14, 2018

Fixes #2533.
Blocked by go-gitea/go-sdk#103.

This PR adds the ability to add due dates to issues. This works similar to milestones, but for single issues. Due date can be managed via the api.
Due dates are shown in the issue list, overdue issues are marked in red.

Screenshots

bildschirmfoto von 2018-04-14 17-19-43
bildschirmfoto von 2018-04-14 17-19-55
bildschirmfoto von 2018-04-14 17-21-12

TODO

  • Update go-sdk once its merged

kolaente added some commits Mar 22, 2018

UpdatedUnix util.TimeStamp `xorm:"INDEX updated"`
ClosedUnix util.TimeStamp `xorm:"INDEX"`
DeadlineUnix util.TimeStamp `xorm:"INDEX"`
DeadlineString string `xorm:"-"`

This comment has been minimized.

@lunny

lunny Apr 15, 2018

Member

It seems this field is unnecessary.

This comment has been minimized.

@kolaente

kolaente Apr 15, 2018

Member

No, it is needed to show the deadline in templates.

This comment has been minimized.

@jonasfranz

jonasfranz Apr 15, 2018

Member

You could use something like .DeadlineUnix.FormatShort.

This comment has been minimized.

@kolaente

kolaente Apr 15, 2018

Member

Thanks, I didn't know that function exists.

On another note, what do you think about putting date formats in the corresponding locale files meaning we could have different date formats for every locale?

}
}

return UpdateIssue(issue)

This comment has been minimized.

@lunny

lunny Apr 15, 2018

Member

this should be err = updateIssue(sess, issue) and need sess.Commit()

This comment has been minimized.

@kolaente

kolaente Apr 15, 2018

Member

Fixed.

kolaente added some commits Apr 15, 2018

@@ -485,6 +491,45 @@ func createAssigneeComment(e *xorm.Session, doer *User, repo *Repository, issue
})
}

func createAddedDeadlineComment(e *xorm.Session, doer *User, repo *Repository, issue *Issue, dateUnix util.TimeStamp) (*Comment, error) {

This comment has been minimized.

@jonasfranz

jonasfranz Apr 15, 2018

Member

I think that you could pack all of these create...DeadlineComment functions into one function with an additional parameter type (please also check if the type is a valid deadline type).

This comment has been minimized.

@kolaente

kolaente Apr 16, 2018

Member

Done, good idea!


// DeadlineForm hold the validation rules for deadlines
type DeadlineForm struct {
DateString string `form:"date" binding:"Required;MaxSize(10)"`

This comment has been minimized.

@jonasfranz

jonasfranz Apr 15, 2018

Member

Replace MaxSize with Size since a yyyy-mm-dd date has always 10 characters.

This comment has been minimized.

@kolaente

kolaente Apr 16, 2018

Member

Thanks, I didn't know Size exists.

<img src="{{.Poster.RelAvatarLink}}">
</a>
<span class="text grey"><a href="{{.Poster.HomeLink}}">{{.Poster.Name}}</a>
{{$.i18n.Tr "repo.issues.due_date_added" .Content $createdStr | Safe}}

This comment has been minimized.

@jonasfranz

jonasfranz Apr 15, 2018

Member

Please remove |Safe since it is not required (no safe HTML content inside)

This comment has been minimized.

@kolaente

kolaente Apr 16, 2018

Member

If I do that, it looks like that:
bildschirmfoto von 2018-04-16 18-47-55

So I'm leaving it. (same for the other ones)

This comment has been minimized.

@jonasfranz

jonasfranz Apr 16, 2018

Member

Fair enough

<img src="{{.Poster.RelAvatarLink}}">
</a>
<span class="text grey"><a href="{{.Poster.HomeLink}}">{{.Poster.Name}}</a>
{{$.i18n.Tr "repo.issues.due_date_modified" .Content $createdStr | Safe}}

This comment has been minimized.

@jonasfranz

jonasfranz Apr 15, 2018

Member

Please remove |Safe since it is not required (no safe HTML content inside)

<img src="{{.Poster.RelAvatarLink}}">
</a>
<span class="text grey"><a href="{{.Poster.HomeLink}}">{{.Poster.Name}}</a>
{{$.i18n.Tr "repo.issues.due_date_remove" .Content $createdStr | Safe}}

This comment has been minimized.

@jonasfranz

jonasfranz Apr 15, 2018

Member

Please remove |Safe since it is not required (no safe HTML content inside)

<form method="POST"{{if gt .Issue.DeadlineUnix 0}}style="display: none;"{{end}}} id="add_deadline_form" action="{{$.RepoLink}}/issues/{{.Issue.Index}}/deadline/update" class="ui action input fluid">
{{$.CsrfTokenHtml}}
<div class="ui fluid action input">
<input required placeholder='{{.i18n.Tr "repo.issues.due_date_form"}}' {{if gt .Issue.DeadlineUnix 0}}value="{{.Issue.DeadlineUnix.Format "2006-01-02"}}"{{end}} type="date" name="date" style="min-width: 13.9rem;border-radius: 4px 0 0 4px;border-right: 0;white-space: nowrap;">

This comment has been minimized.

@jonasfranz

jonasfranz Apr 15, 2018

Member

Replace placeholder='...' with placeholder="..."

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"

This comment has been minimized.

@jonasfranz

jonasfranz Apr 15, 2018

Member

I think that

format yyyy-mm-dd

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 yyyy-mm-dd. (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/date#Value)

This comment has been minimized.

@kolaente

kolaente Apr 16, 2018

Member

Yes, but that value is used as a placeholder for the input. If the browser supports input type date, the browser does what you described thus not showing the placeholder at all (because there's already something inside that field).

Some screenshots with an empty input:

Firefox:
bildschirmfoto von 2018-04-16 18-42-51

Chromium:
bildschirmfoto von 2018-04-16 18-43-22

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.

This comment has been minimized.

@jonasfranz

jonasfranz Apr 16, 2018

Member

If the placeholder isn't shown at all it could be removed to reduce translation effort.

This comment has been minimized.

@kolaente

kolaente Apr 16, 2018

Member

It is shown when the browser doesn't support the <input type="date"/> tag, so I'd leave that.

kolaente added some commits Apr 16, 2018

Small improvments to comment creation
+ ui & validation improvements
// AfterLoad checks if the issue is overdue and sets the property
func (issue *Issue) AfterLoad() {
if util.TimeStampNow() >= issue.DeadlineUnix {
issue.IsOverDue = true

This comment has been minimized.

@lunny

lunny Apr 17, 2018

Member

why not just change IsOverDue as a method of Issue?

This comment has been minimized.

@kolaente

kolaente Apr 17, 2018

Member

Good idea, I've changed it.

kolaente added some commits Apr 17, 2018

kolaente added some commits Apr 29, 2018

Merge branch 'master' of https://github.com/go-gitea/gitea into issue…
…-due-date

# Conflicts:
#	models/issue.go
#	modules/templates/helper.go
#	options/locale/locale_en-US.ini
if err := sess.Begin(); err != nil {
return err
}
defer sess.Close()

This comment has been minimized.

@Morlinest

Morlinest Apr 29, 2018

Member

defer sess.Close() should be after sess := x.NewSession() and before sess.Begin()

This comment has been minimized.

@kolaente

kolaente Apr 29, 2018

Member

Sure, fixed.

@@ -265,6 +272,16 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
issue.Content = *form.Body
}

var deadlineUnix util.TimeStamp

This comment has been minimized.

@Morlinest

Morlinest Apr 29, 2018

Member

nit: Use var deadlineUnix util.TimeStamp or var deadline util.TimeStamp, not both as it has same meaning (same in repo/issue.go and repo/pull.go).

This comment has been minimized.

@kolaente

kolaente Apr 29, 2018

Member

Sure, I fixed that as well.

kolaente added some commits Apr 29, 2018

}

// Check if the user has at least write access to the repo
if !ctx.Repo.IsWriter() {

This comment has been minimized.

@Morlinest

Morlinest Apr 29, 2018

Member

I found there is authorization middleware, can you use that instead? Like in m.Post("/status", reqRepoWriter, repo.UpdateIssueStatus). It calls RequireRepoWriter() func.

This comment has been minimized.

@kolaente

kolaente added some commits Apr 29, 2018

@jonasfranz

This comment has been minimized.

Copy link
Member

jonasfranz commented May 1, 2018

@lunny I already approved it here: #3794 (comment)

@Morlinest
Copy link
Member

Morlinest left a comment

Should be last changes. There are another two things I don' like. But one is not related directly only to this issue (usage of ctx.Writen() and ctx.HasError()) and second is about better UI (but that can be updated in future, right now I have not better idea).

@@ -496,6 +496,8 @@ func RegisterRoutes(m *macaron.Macaron) {
})
})
m.Post("/reactions/:action", bindIgnErr(auth.ReactionForm{}), repo.ChangeIssueReaction)
m.Post("/deadline/update", reqRepoWriter, bindIgnErr(auth.DeadlineForm{}), repo.UpdateDeadline)
m.Post("/deadline/remove", reqRepoWriter, repo.RemoveDeadline)

This comment has been minimized.

@Morlinest

Morlinest May 1, 2018

Member

For consistency, this endpoint should be called /delete.

This comment has been minimized.

@kolaente

kolaente May 1, 2018

Member

Fixed that.

{{end}}
<br/>
<a style="cursor:pointer;" onclick="toggleDuedateForm();"><i class="edit icon"></i>Edit</a> -
<a style="cursor:pointer;" onclick="deleteDueDate('{{$.RepoLink}}/issues/{{.Issue.Index}}/deadline/remove');"><i class="remove icon"></i>Remove</a>

This comment has been minimized.

@Morlinest

Morlinest May 1, 2018

Member

This can/should be standard link (<a href="{{$.RepoLink}}/issues/{{.Issue.Index}}/deadline/remove">), right?

This comment has been minimized.

@kolaente

kolaente May 1, 2018

Member

No, the js function sends a post request.

This comment has been minimized.

@kolaente

kolaente May 1, 2018

Member

I'll send a seperate pr after this one is merged which will add a seperate api endpoint for deadlines which then will be used for the update the deadline from the UI.

This comment has been minimized.

@Morlinest

Morlinest May 1, 2018

Member

Good to know you will improve this solution and add api endpoint. Until then, you should not simulate api endpoint by using js and standard page, ignoring anything it can return. For now, there is not reason to use js, you can add it later, in another PR when api would be ready for it.

This comment has been minimized.

@kolaente

kolaente May 1, 2018

Member

I've modifed the endpoint, deleting a deadline is now done via GET.

kolaente added some commits May 1, 2018

@Morlinest
Copy link
Member

Morlinest left a comment

gj

@kolaente

This comment has been minimized.

Copy link
Member

kolaente commented May 1, 2018

@Morlinest thanks!

@lafriks
Copy link
Member

lafriks left a comment

@Morlinest @kolaente Wanted to already merge but there one thing needs to be changed back as it was in previous commits. For security reasons all actions that update database must be done using post method, so that CSRF can be verified and user could not be fooled into actions that he did not want (either by giving him url or redirecting him to such url)

@kolaente

This comment has been minimized.

Copy link
Member

kolaente commented May 1, 2018

@lafriks OK, I'll change it back to how it was before.

@kolaente

This comment has been minimized.

Copy link
Member

kolaente commented May 1, 2018

@lafriks Done.

@lafriks

lafriks approved these changes May 1, 2018

@lafriks lafriks merged commit 1a97030 into go-gitea:master May 1, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@lafriks lafriks added the changelog label May 1, 2018

@Morlinest

This comment has been minimized.

Copy link
Member

Morlinest commented May 1, 2018

Just for info, I don't like this halfway solution.

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented May 1, 2018

@Morlinest about doing post for removing due date?

@kolaente kolaente referenced this pull request May 2, 2018

Merged

Issue due date api #3890

jonasfranz added a commit to jonasfranz/gitea that referenced this pull request May 6, 2018

Issue due date (go-gitea#3794)
* Started adding deadline to ui

* Implemented basic issue due date managing

* Improved UI for due date managing

* Added at least write access to the repo in order to modify issue due dates

* Ui improvements

* Added issue comments creation when adding/modifying/removing a due date

* Show due date in issue list

* Added api support for issue due dates

* Fixed lint suggestions

* Added deadline to sdk

* Updated css

* Added support for adding/modifiying deadlines for pull requests via api

* Fixed comments not created when updating or removing a deadline

* update sdk (will do properly once go-gitea/go-sdk#103 is merged)

* enhanced updateIssueDeadline

* Removed unnessecary Issue.DeadlineString

* UI improvements

* Small improvments to comment creation
+ ui & validation improvements

* Check if an issue is overdue is now a seperate function

* Updated go-sdk with govendor as it was merged

* Simplified isOverdue method

* removed unessecary deadline to 0 set

* Update swagger definitions

* Added missing return

* Added an explanary comment

* Improved updateIssueDeadline method so it'll only update `deadline_unix`

* Small changes and improvements
* no need to explicitly load the issue when updating a deadline, just use whats already there
* small optimisations

* Added check if a deadline was modified before updating it

* Moved comment creating logic into its own function

* Code cleanup for creating deadline comment

* locale improvement

* When modifying a deadline, the old deadline is saved with the comment

* small improvments to xorm session handling when updating an issue deadline
+ style nitpicks

* style nitpicks

* Moved checking for if the user has write acces to middleware
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment