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

Add UI to delete tracked times #14100

Merged
merged 40 commits into from
Feb 19, 2021
Merged

Add UI to delete tracked times #14100

merged 40 commits into from
Feb 19, 2021

Conversation

noerw
Copy link
Member

@noerw noerw commented Dec 21, 2020

Adds a button on time tracker issue comments to delete a tracked time again (in an append-only-log style).
This button is available only to the poster of the tracked time and site admins.

Caveat is, that this does not work for times created until now, as the comment has no reference to the time ID. (This could probably be resolved once via a migration that mactches comments & times with common fields, but thats far out of my comfort zone)

I'm not very familiar with gitea codebase yet, so better check things twice.
In particular I don't know if a migration is needed for the fields added to Comment..

Peek 2020-12-22 14-26

fixes #2642

@noerw noerw added the type/enhancement An improvement of existing functionality label Dec 21, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 21, 2020
routers/repo/issue.go Outdated Show resolved Hide resolved
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

this code will never be used ... - change it

routers/repo/issue_timetrack.go Outdated Show resolved Hide resolved
routers/repo/issue_timetrack.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot 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, 2020
@6543 6543 added this to the 1.14.0 milestone Dec 22, 2020
Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

It should ask for confirmation in dialog if you really want to delete it

@noerw
Copy link
Member Author

noerw commented Dec 22, 2020

It should ask for confirmation in dialog if you really want to delete it

@lafriks done!

@codecov-io
Copy link

codecov-io commented Dec 22, 2020

Codecov Report

Merging #14100 (18d62ef) into master (91e59a6) will increase coverage by 0.00%.
The diff coverage is 27.50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14100   +/-   ##
=======================================
  Coverage   41.76%   41.77%           
=======================================
  Files         748      749    +1     
  Lines       80103    80143   +40     
=======================================
+ Hits        33458    33476   +18     
- Misses      41123    41147   +24     
+ Partials     5522     5520    -2     
Impacted Files Coverage Δ
models/migrations/migrations.go 2.28% <ø> (ø)
models/migrations/v167.go 0.00% <0.00%> (ø)
routers/repo/issue_timetrack.go 0.00% <0.00%> (ø)
models/issue_comment.go 52.46% <71.42%> (+0.19%) ⬆️
models/issue_stopwatch.go 71.68% <100.00%> (+0.25%) ⬆️
models/issue_tracked_time.go 69.93% <100.00%> (+0.18%) ⬆️
routers/repo/issue.go 38.48% <100.00%> (+0.11%) ⬆️
routers/routes/macaron.go 93.27% <100.00%> (+<0.01%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.54%) ⬇️
... and 8 more

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 9872b8d...18d62ef. Read the comment docs.

web_src/js/index.js Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

The use of a rounded button seems a bit odd to me. Don't we already have rectangular buttons in use in the history somewhere? Would prefer a rectangular button with text and maybe a icon.

routers/routes/web.go Outdated Show resolved Hide resolved
@delvh
Copy link
Member

delvh commented Feb 14, 2021

If I see correctly what this PR is doing, there is currently no support to revert time using the API, right?
Should an issue be opened for that as soon as this branch gets merged?

@lunny
Copy link
Member

lunny commented Feb 15, 2021

@lafriks Need your review again.

@6543
Copy link
Member

6543 commented Feb 15, 2021

@delvh
Copy link
Member

delvh commented Feb 15, 2021

@6543 wait, it's already supported for the API but not for the web interface? Strange…

@lunny
Copy link
Member

lunny commented Feb 18, 2021

See #14100 (comment), otherwise LGTM.

@GiteaBot GiteaBot 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 Feb 18, 2021
@6543
Copy link
Member

6543 commented Feb 19, 2021

@lunny done

@lafriks lafriks merged commit d38ae59 into go-gitea:master Feb 19, 2021
@6543 6543 deleted the delete-times-ui branch February 19, 2021 11:07
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
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/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to edit and delete Time Tracking entries
8 participants