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

timestat back and forth is weird #114

Closed
sductor opened this issue Dec 29, 2017 · 15 comments
Closed

timestat back and forth is weird #114

sductor opened this issue Dec 29, 2017 · 15 comments
Labels

Comments

@sductor
Copy link

sductor commented Dec 29, 2017

Hi,

First thank you for the work.
My issue is that I find the behavior of a certain function weird.
But maybe this is not a problem with the code but I didn't unerstand how I am supposed to use it.
Here the faulty code:

iapi = new GitLabApi(…).createIssue(26, "test", "yo");
val issue = iapi.createIssue(26, "test", "yo");
iapi.estimateTime(issue.getProjectId(), issue.getIid(), "3mo 2w");

At this point an issue is created but it displays an estimated time of 5 months!

Then

println(org.gitlab4j.api.models.Duration(api.getTimeTrackingStats(issue.getProjectId(), issue.getIid()).getTimeEstimate())))

display 1mo3d8h

Thus I finnd the utilisation really not intuitive
It appears to me that the problem come from the fact that you are using real time value whereas gitlab use a day = 8h etc. But as this is an api for gitlab, I found a pity that you does not use the same standard as gitlab. I imagine that I would have to do a adapter that would go back and forth between your standard and the one of gitlab. But I think this is also a pity since you already ave those variables
as the two first lines of DurationUtils but that are private.

I feel that the project would benefit to:

  • adding a second TIME_UNIT_MULTIPLIERS that would meet with gitlab
  • use a singleton pattern for accessing to DurationUtil rather than using the plain class
  • Parameterize this singleton to choose which unit multiplier it should uses
  • Allow to define this parameter (set) in the class Duration
@gmessner
Copy link
Collaborator

gmessner commented Dec 29, 2017

@sductor
You've simply discovered a bug, GitLab4J-API should be using the same exact time units as GitLab. There is no need allow the selection of the of a different implementation, this implementation needs to be corrected so that it is 100% in line with GitLab. It will be fixed in the next day or so as it is a fairly significant bug.

@gmessner
Copy link
Collaborator

@sductor
Can you confirm the following breakdown?
60s = 1m
60m = 1h
8h = 1d
5d = 1w
4w = 1mo

@sductor
Copy link
Author

sductor commented Dec 29, 2017

Yep it is what I got on
myserver/help/workflow/time_tracking.md
!

@gmessner
Copy link
Collaborator

@sductor
My concern is the final section called "Configuration", it implies that the number of hours in a day can be changed, but I am unable to locate where that is set, do you know?

@sductor
Copy link
Author

sductor commented Dec 29, 2017

I don't think it is possible and would be surprised if it was…
Such a configuration would be really error prone… So far as I've searched I did not find anything
Anyway, maybe you can just let a comment on the code if this feature appear in a next version and someone want to use it. Then my singleton approach would make sense… But I don't think it will be usefull now

@gmessner
Copy link
Collaborator

@sductor
If GitLab ever allows the changing of hours in a day (both JIRA and TFS allow it), then they will need to pass that setting around with the time estimates or things will very error prone (as you also believe). Passing the hours/day with the time estimate would eliminate the need for a singleton implementation, it would simply be passed as a parameter when parsing and formatting.

I have made the fix and have released version 4.7.8.

@gmessner gmessner added the bug label Dec 29, 2017
@sductor
Copy link
Author

sductor commented Jan 5, 2018

I want to reopen the issue, since it seems to have other problems.
Here is the snwe:
'''
val iapi = (…).getIssuesApi()
val issue = iapi.createIssue(31, "test", "yo");
iapi.estimateTime(issue.getProjectId(), issue.getIid(), "2mo");
println(org.gitlab4j.api.models.Duration(iapi.getTimeTrackingStats(issue.getProjectId(), issue.getIid()).getTimeEstimate()));
'''

Whenever I read the issue from gitlab the time is accurate.
However if I create an issue with a time including mo, it will create an issue of 1month and 2 week for each month. Hence creating an issue of 1 month and 1 day results in an issue of 1 month and 2 week and one day and an issue of 2 months rand 1 day results in an issue of 3 months and 1 day.
This affects only month…

@gmessner gmessner reopened this Jan 5, 2018
@gmessner
Copy link
Collaborator

gmessner commented Jan 5, 2018

You have discovered an actual bug with GitLab, not this library. When I make time_estimate calls it looks like the set time estimate endpoint considers "1 month" = "30 days". I have validated this by calling the GitLab API directly using Postman.

As you have discovered, if the "mo" (month) duration is used when setting the time_estimate, GitLab will set the time_estimate to "1mo 2w" for every "1mo", so you get the following results:

Set Duration Result
1mo 1mo 2w
2mo 3mo
3mo 4mo 2w
4mo 6mo
1mo 1w 1mo 3w
1mo 2w 2mo

If I do not use the "mo" duration, everything is fine:

Set Duration Result
20d 1mo
30d 1mo 2w
4w 1mo
6w 1mo 2w
... ...

The bug with GitLab is that when setting duration, "1mo" = "30d", but when fetching duration "1mo" = "4w" (20 days). What is interesting, is they do not even reference the "mo" duration in the docs at:

myserver/help/workflow/time_tracking.md

@sductor
Copy link
Author

sductor commented Jan 6, 2018

I think I have understand the problem.
It seems that you solved it in the last commit, even if I did not understand how. ^^"
Anyway thank you for your hard work
I will wait for the next release on Maven to test all this, as I'm not familiar with compilation from source

@sductor sductor closed this as completed Jan 6, 2018
gmessner added a commit that referenced this issue Jan 6, 2018
@sductor
Copy link
Author

sductor commented Jan 7, 2018

For the record, it seems that you lost the "day" precision.
2mo plus 1 to 4 days is uploaded as 2mo
2mo plus 5 days is uploaded as 2mo1w
The same behavior seems to appear for weeks

As far as I tested, everything seems fine
Thanks for the work

@gmessner gmessner reopened this Jan 7, 2018
@sductor
Copy link
Author

sductor commented Jan 7, 2018

In case you are interested, here is an unofficial imperative adaptation of the «laws» defined by haskell to ensure the validity of the implementation of a «getter/setter» object (lens)
This exploits a model of approach of test that is implemented by junit using "@theory" if I remember well:

  • forall x, set(x) then get() return x
  • if get() return x, set(get()) then get() return x
  • forall x, set(x) then set(x) then get() return x
    («then» means «;»)

The idea is to implement this three rules and a generator of time examples and to check for each time example generated if the rules are verified.

@gmessner
Copy link
Collaborator

gmessner commented Jan 7, 2018

@sductor
You are right, this time the bug was in this package. If there were weeks but no hours, minutes, or seconds days were dropped. I have fixed this, added unit tests (with round trip) for same and released 4.7.12

I'll have to look into your suggestion, but in a sense have caught it with the round trip tests.

@gmessner gmessner closed this as completed Jan 7, 2018
gmessner added a commit that referenced this issue Jan 7, 2018
@sductor
Copy link
Author

sductor commented Jan 7, 2018

my "suggestion" was more a genuine share of knowledge than anything else.
feel free to do whatever you want with it =)

Thanks for the work

@gmessner
Copy link
Collaborator

gmessner commented Jan 7, 2018

@sductor
If you can please let me know if everything is as it should be. This one proved to be an interesting set of problems, with issues on both sides.

I still need to submit a bug with GitLab to let them know that they have an issue with time_estimate using 30 days per month, and everywhere else it is 20 days per month.

@sductor
Copy link
Author

sductor commented Jan 8, 2018

As far as I observed, everything seems good.

The ultimate step of my project was to add a duration for a starting date to obtain the ending date.
Use case is that I have the starting time of an issue, the estimate time and I want to obtain the ending time.
As duration are handled now with «working time» it could not be used directly through Calender class.
I've made some function to do this operation. There are in scala, but I'm sharing the code with you as it can be usefull for your project. I've done some test using the well-known «grep with my eyes» approach, thus no guarantee of working ^^"

def workTimeToRealTime(seconds : Integer) : Integer = {
    val workdaytime : Integer  = 60 * 60 * 8 // 8 hours = 1 day
    val workweektime : Integer = workdaytime * 5   // 5 days = 1 week

    val realdaytime : Integer = 60 * 60 * 24
    val realweektime : Integer = realdaytime * 7

    val nbweeks : Integer = seconds / workweektime
    val nbdays : Integer = (seconds % workweektime) / workdaytime
    val reminds : Integer = seconds % workweektime % workdaytime

    val realweeks : Integer = nbweeks * realweektime
    val realdays : Integer = nbdays * realdaytime

    return realweeks + realdays + reminds
  }
def add(d : Date, workseconds : Integer) : Date = {
   val c = Calendar.getInstance()
   c.setTime(d)
   c.add(Calendar.SECOND, workTimeToRealTime(workseconds))
   c.getTime()
 }

Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants