implemented add custom date to spent time #16870
Conversation
| @@ -804,8 +804,9 @@ func (f *DeleteRepoFileForm) Validate(req *http.Request, errs binding.Errors) bi | |||
|
|
|||
| // AddTimeManuallyForm form that adds spent time manually. | |||
| type AddTimeManuallyForm struct { | |||
| Hours int `binding:"Range(0,1000)"` | |||
| Minutes int `binding:"Range(0,1000)"` | |||
| Created string `form:"created" binding:"OmitEmpty;Size(10)"` | |||
There was a problem hiding this comment.
That will fail after the year 9999
| <div class="header">{{.i18n.Tr "repo.issues.add_time"}}</div> | ||
| <div class="content"> | ||
| <form method="POST" id="add_time_manual_form" action="{{$.RepoLink}}/issues/{{.Issue.Index}}/times/add" class="ui action input fluid"> | ||
| {{$.CsrfTokenHtml}} | ||
| <input type="date" name="created"> |
There was a problem hiding this comment.
I think it would be a good idea to add pattern="\d{4,}-\d{2}-\d{2}" according to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/date#handling_browser_support. Otherwise, a correct date could be added in an incorrect format that would then subsequently be reset to today.
Additionally, we could even think about using a maximum value of "today" or "tomorrow", as future times should in my opinion not be possible to be added. (Maybe except for timezones, hence why I would argue to leave "tomorrow" as a valid option). The problem with that is that the HTML standard does not support "today" or "tomorrow" as values and hence these values would need to be inserted on the server-side.
Thoughts on the maximum value?
There was a problem hiding this comment.
Regrading your proposal to add pattern="...". I am not sure that is necessary to do. I look through Yandex Browser, Firefox, Google Chrome, Opera, Safari. Default behaviour of browsers while adding input type=date looks reasonable. I can't add incorrect date and can't add extra range values.
Yandex Browser
Firefox
Google Chrome
Opera
Safari
Second part of your review to use maximum value "today" or "tomorrow" is a good proposal. I try to find how to implement it and add it to this PR.
| @@ -20,7 +20,7 @@ type TrackedTime struct { | |||
| UserID int64 `xorm:"INDEX"` | |||
| User *User `xorm:"-"` | |||
| Created time.Time `xorm:"-"` | |||
| CreatedUnix int64 `xorm:"created"` | |||
| CreatedUnix int64 `xorm:"created_unix"` | |||
There was a problem hiding this comment.
why do you rename the table column in db there is no need to do so
PS: migrations would be missing too - but just drop that change
There was a problem hiding this comment.
I am not. Looks like this is an error in xorm.
This is what i have in sqlite db.
sqlite> .dump tracked_time
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE tracked_time (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, issue_id INTEGER NULL, user_id INTEGER NULL, created_unix INTEGER NULL, time INTEGER NOT NULL, deleted INTEGER DEFAULT 0 NOT NULL);
There was a problem hiding this comment.
The created here is a XORM keyword, not a column name. The keyword shouldn't be changed.
So the column name is CreatedUnix => created_unix (implicitly), it was correct already.
|
there are two option to handle it in the backend: either allow the user to optional specify arbitrary creation timestamps to be inserted into db (not fan of it) or to add a new column who would be either NULL or specify when the spent time should be "proposed" to be happened |
Thank you for your review and proposals.
Any thoughts regarding this? |
Implemented issue #16841