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.

Sign up for GitHub

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

Jump to bottom

Make tracked time representation display as hours #33315

Merged
merged 5 commits into from
Jan 19, 2025
Merged

Make tracked time representation display as hours #33315

merged 5 commits into from
Jan 19, 2025
+29 −94

Conversation

vsysoev
Copy link
Contributor

@vsysoev vsysoev commented Jan 17, 2025 •
edited by wxiaoguang
Loading

Since #25213 is not moving forward, I propose another minimal approach to change tracked time representation.

Tracked time in the issue represented not in the same manner as estimated.

Because of estimated time represented in hours it might be convenient to have tracked time represented in the same way to be compared and managed.

(oudated)

Option TRACKED_TIMESTAMP_TENSE added to [ui] section.

[ui]
TRACKED_TIMESTAMP_TENSE='mixed|hours'

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 17, 2025
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 17, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jan 17, 2025
@vsysoev vsysoev force-pushed the tracked-time-representation branch from 39ad067 to e184c21 Compare January 17, 2025 19:22
@wxiaoguang
Copy link
Contributor

Maybe it should be a per-user setting? It only affects the display, so it's user's preference?

@vsysoev
Copy link
Contributor Author

vsysoev commented Jan 18, 2025 •
edited
Loading

Maybe it should be a per-user setting? It only affects the display, so it's user's preference?

@wxiaoguang, thank you for your review. My points are:

  • Main: Estimation/tracked (time or points) values should be in the same view mode to be comparable. They are used to define team/contributor performance and planning poker.
  • Existing approach to show time in gitea. There is UI.PREFERED_TIMESTAMP_TENSE option exists. My PR based on the same approach.
  • Usage model in my company is standalone installations for the dev teams. It is not cloud installation for the unpredictable amount of single or small teams contributors.

I am personally definitely prefer that default behaviour of estimated/tracked time representation will be in "hours". Because this representation elliminates many differences with shifts, days, WW and other managements stuffs.

@wxiaoguang
Copy link
Contributor

Thank you for the explanation. TBH if there is a chance to avoid a new global config option, I think it's better to do that, meanwhile I won't block the current approach.

But I can see there are other related PRs:

And @6543 to take a look at this PR and #25213

@wxiaoguang wxiaoguang mentioned this pull request Jan 18, 2025
Open
9 tasks
@vsysoev
Copy link
Contributor Author

vsysoev commented Jan 18, 2025 •
edited
Loading

Thank you for the explanation. TBH if there is a chance to avoid a new global config option, I think it's better to do that, meanwhile I won't block the current approach.

But I can see there are other related PRs:

And @6543 to take a look at this PR and #25213

Yes, I took a look on #25213 and did several comments in this PR. But #25213 isn't moving forward for more than a year.
#28312 is my "old patch" which I use for my installations release 1.20+. In 1.23.1 I found than "estimated" field have been added. So I see that #28312 is outdated and I close it. I reimplement patch for my needs and propose it to upstream.

IMHO, default behaviour should be changed as I've written in previous comment. Estimated time and tracked time should be shown in hours to be comparable and minimise cultural and management differences around the World.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 18, 2025 •
edited
Loading

Maybe it is a good chance to completely remove the legacy SecToTime, it doesn't make sense to convert 24hours to one day. Then no need to introduce the new option.

@vsysoev
Copy link
Contributor Author

vsysoev commented Jan 18, 2025

Maybe it is a good chance to complete remove the legacy SecToTime, it doesn't make sense to convert 24hours to one day. Then no need to introduce the new option.

If it will be suitable I remove SecToTime and replaced it with SecToHours.

@vsysoev vsysoev force-pushed the tracked-time-representation branch from e184c21 to d8a50df Compare January 18, 2025 13:05
@vsysoev
Tracked time representation changed to hours
fa65e25 
Tracked time in the issue represented not in the same manner as
estimated.

Because of estimated time viewed in hours it might be convinient to have
tracked time represented in the same way.
Signed-off-by: Sysoev, Vladimir <[email protected]>
@vsysoev vsysoev force-pushed the tracked-time-representation branch from d8a50df to fa65e25 Compare January 18, 2025 13:06
@wxiaoguang wxiaoguang changed the title Tracked time representation (added hours) might be modified with settings in UI section in app.ini Tracked time representation (added hours) is displayed as hours Jan 18, 2025
@wxiaoguang wxiaoguang changed the title Tracked time representation (added hours) is displayed as hours Make tracked time representation display as hours Jan 18, 2025
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 18, 2025
@wxiaoguang
Copy link
Contributor

We also need to fix frontend: f333786

@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 Jan 18, 2025
@wxiaoguang wxiaoguang added this to the 1.24.0 milestone Jan 18, 2025
@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 Jan 18, 2025
@wxiaoguang wxiaoguang merged commit dc2308a into go-gitea:main Jan 19, 2025
26 checks passed
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 20, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
5f079b0 
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Fix typo in documentation of GetBranchNames() (go-gitea#33330)
  Fix Account linking page (go-gitea#33325)
  Fix parentCommit invalid memory address or nil pointer dereference. (go-gitea#33204)
  Make tracked time representation display as hours (go-gitea#33315)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  Fix push message behavior (go-gitea#33215)
  Add golang back to nix dev environment and pin v1.23 (go-gitea#33275)
  Trivial fixes (go-gitea#33304)
@wxiaoguang wxiaoguang mentioned this pull request Jan 20, 2025
Open
wxiaoguang pushed a commit to wxiaoguang/gitea that referenced this pull request Jan 20, 2025
@vsysoev @wxiaoguang
Make tracked time representation display as hours (go-gitea#33315)
09f6c74 
Estimated time represented in hours it might be convenient to
have tracked time represented in the same way to be compared and
managed.

---------

Co-authored-by: Sysoev, Vladimir <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
# Conflicts:
#	modules/templates/helper.go
@wxiaoguang wxiaoguang mentioned this pull request Jan 20, 2025
Merged
lunny pushed a commit that referenced this pull request Jan 20, 2025
@wxiaoguang
Make tracked time representation display as hours (#33315) (#33334)
eef6355 
Try to backport #33315, the only trivial conflict is in the helper
functions map in the helper.go

Fix #33333

Co-authored-by: Sysoev, Vladimir <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.
4 participants
@vsysoev @wxiaoguang @lunny @GiteaBot