Add the changed time in the note webhook at least in a machine readable format

Description

This feature proposal is extracted from this bigger ticket: gitlab-ce#33211. You may also want to check this original ticket if some context is lacking.

We are in the process of migrating our Trac setup to GitLab EES. We are heavily using time tracking internally, and our main use case is that when anything that has a time field happen in Trac, a webhook is triggered in our (custom) CRM to generate (1) a feed of events showing the comment and time tracked , and (2) an aggregate of the time tracked to calculate our hours worked and overtime as soon as an event happen.

Right now, the webhook triggered when a note is added, only contained the aggregated time on the whole ticket like so:

{
...
    "total_time_spent": 72000,
    "human_total_time_spent": "2d 4h",
...
}

Proposal

Our proposal would be to add at least a machine-readable field containing time that got tracked. A human readable field could also be added for convenience, but the machine readable one is actually the most important of the two to digest the webhook easily:

{
...
    "total_time_spent": 72000,
    "human_total_time_spent": "2d 4h",
    "time_change": 600,
    "human_time_change": "10m",
...
}

The advantage of a setup like this is that one could easily plug any time tracking system that can react on webhook or API trigger. The fact that the field is machine-readable also help simplify the parsing of such information.

@Mayeu Thanks for the proposal!

changed title from Proposal: add the changed time in the note webhook at least in a machine readable format to Add the changed time in the note webhook at least in a machine readable format

Hello,

Any news about the prioritisation of this proposal? From my interaction via GitLab's Support it seems you were waiting for your next internal meeting to see about this.

Also, if there is anything we can do to help move this forward don't hesitate to ping us about it :)

So this proposal is to deliver the difference between the old and new values for consumers that don't store the original time spent?

Yes. Since the webhook contains what changed when an event happen (I use the event here for any user input changing the state of the issue tracker), it would make sense to provide not only the new state, but the changed delta.

This will also simplify using tools in a more plug-n-play fashion. One can just hook-up a tool to the hook, and start gathering interesting data without having to know or import the state of the system beforehand.

Related to this issue I created gitlab-ce#40122 which also prevent/make it hard to integrate the time tracking feature of GitLab.

Following the comments on gitlab-ce#40122 about future change of GitLab 10.2, I am currently downloading the last RC to check if that resolve our business case.

added gitlab-ce3857529a gitlab-ce3713901 and removed ~1672339 labels

added Create [DEPRECATED] label

added [deprecated] Accepting merge requests label

added devopsplan groupproject management labels

removed [deprecated] Accepting merge requests label

mentioned in issue #212102 (closed)

mentioned in issue #212794 (closed)

mentioned in issue #212807 (closed)

mentioned in issue #213483 (closed)

mentioned in issue #226940 (closed)

mentioned in issue #230638 (closed)

mentioned in issue #232360 (closed)

mentioned in issue #233315 (closed)

mentioned in issue #235070 (closed)

mentioned in issue #237808 (closed)

mentioned in issue #240839 (closed)

mentioned in issue #243452 (closed)

added sectiondev label

mentioned in issue #246443 (closed)

mentioned in issue #249045 (closed)

mentioned in issue #254146 (closed)

mentioned in issue #257772 (closed)

mentioned in issue #261979 (closed)

mentioned in issue gitlab-org/quality/triage-reports#473 (closed)

mentioned in issue gitlab-org/quality/triage-reports#533 (closed)

mentioned in issue gitlab-org/quality/triage-reports#609 (closed)

mentioned in issue gitlab-org/quality/triage-reports#674 (closed)

mentioned in issue gitlab-org/quality/triage-reports#707 (closed)

mentioned in issue gitlab-org/quality/triage-reports#797 (closed)

mentioned in issue gitlab-org/quality/triage-reports#874 (closed)

mentioned in issue gitlab-org/quality/triage-reports#949 (closed)

mentioned in issue gitlab-org/quality/triage-reports#1050 (closed)

mentioned in issue gitlab-org/quality/triage-reports#1104 (closed)

mentioned in issue gitlab-org/quality/triage-reports#1193 (closed)

mentioned in issue gitlab-org/quality/triage-reports#1277 (closed)

mentioned in issue gitlab-org/quality/triage-reports#1372 (closed)

mentioned in issue gitlab-org/quality/triage-reports#1437 (closed)

added to epic &5147

mentioned in issue gitlab-org/quality/triage-reports#1515 (closed)

This would be a great Community contribution and fairly simple to do. If we extend this piece (https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/concerns/issuable.rb#L465) to add a new attribute for the change between the old and new times, we can determine the time_change!

added [deprecated] 🍎 backend workflowready for development labels

set weight to 1

removed Create [DEPRECATED] label

removed Plan [DEPRECATED] label

mentioned in issue gitlab-org/quality/triage-reports#1601 (closed)

mentioned in issue gitlab-org/quality/triage-reports#1690 (closed)

mentioned in issue #321207 (closed)

Hey, I'm looking forward to work on this. I just want to clarify if I'm getting the issue right.

if user spend 1h30m and then spend 2h. i.e.

/spend 1h30m
/spend 2h

Then we want our webhook response to look something like this:

{
...
    "total_time_spent": 12600,
    "time_change": 7200,
    "human_total_time_spent": "3h 30m",
    "human_time_change": "2h",
...
}

correct?

cc: @acroitor @johnhope @cablett

@harry-hov yes correct, that is my understanding as well based on above comments:

So this proposal is to deliver the difference between the old and new values for consumers that don't store the original time spent?

Yes. Since the webhook contains what changed when an event happen (I use the event here for any user input changing the state of the issue tracker), it would make sense to provide not only the new state, but the changed delta.

Thanks for your interest and contribution!

Mine too, thanks for the contribution @harry-hov. Please assign yourself when you're ready to start work.

/cc @jlear

Thanks for confirming @acroitor, @johnhope !

I have few more queries those are maybe worth discussing:

1. Unlike total_time_spent, time_change can be negative. How do we want to represent -ve time_change in human understanding format? maybe like "-1h30m"? I'm not sure.

2. What if total_time_spend becomes 0?.

Here comes the 2 possible cases:

Case 1:

/spend 1h
/spend -1h 

In that case current webhook response looks something like this:

{
...
    "total_time_spent": 0
    "human_total_time_spent": null
...
}

What should be the value of time_change in this case? -3600 ?

Case 2:

/spend 1h
/remove_time_spent

In that case current webhook response looks something like this:

{
...
    "total_time_spent": 0
    "human_total_time_spent": null
...
}

What should be the value of time_change in this case? -3600 ? or we need to reset the time_change to 0?

Mine too, thanks for the contribution @harry-hov. Please assign yourself when you're ready to start work.

@johnhope I'm ready to start working on this. Can you please assign this to me? IDK how can I assign myself. Thanks.

Can you please assign this to me? IDK how can I assign myself. Thanks.

@harry-hov you can assign it from sidebar assign yourself link or use a quick action comment /assign @harry-hov in a comment

Thanks @acroitor for your response. I tried and I guess /assign @harry-hov isn't working for me. I'm not sure if I have the right permission to do this?

human understanding format? maybe like "-1h30m"

looks right to me

@harry-hov I believe we would allow negative values for both time_change and human_tim_change, e.g.

    "time_change": -7200
    "human_total_time_spent": "3h 30m"
    "human_time_change": "-2h"

What if total_time_spend becomes 0?.

either 0 or null for human_total_time_spent sounds good to me, I would prefer 0 tbh, but do not have a strong opinion.

For the case where the substracted value is bigger than the value of total_time_spent we still want to return the delta, e.g.

initial data:

    "total_time_spent": 12600
    "human_total_time_spent": "3h 30m"

remove over what has been spent

/spend -5h

returns:

    "total_time_spent": 0
    "human_total_time_spent": "0"
    "time_change": -12600
    "human_time_change": "-3h 30m"

For the case where the substracted value is bigger than the value of total_time_spent we still want to return the delta,

@acroitor

value of total_time_spent remains untouched if substracted value is bigger than the actual value of `total_time_spent.

e.g.

Input:

/spend 1h
/spend -2h

Webhook final Response:

    "total_time_spent": 3600
    "human_total_time_spent": "1h"

Intended? or bug?

ok, this seems to be expected behaviour based on the documentation

You can remove time by entering a negative amount: for example, /spend -3d removes three days from the total time spent. You can’t go below 0 minutes of time spent, so GitLab automatically resets the time spent if you remove a larger amount of time compared to the time that was entered already.

@acroitor Really? Is this intended?

I would expect that if I have previously spent 1hr and I then spend -2hr, the total time spent would be 0.

@hollyreynolds @gweaver maybe we could break this out as a separate bug.

@harry-hov I think it's fine to press on with returning the delta from the actual change in total time spent. So if that is currently 0, time_change should be 0 also. Even if it's unexpected behaviour.

@johnhope it is documented so I guess it is intended,

@acroitor It just didn't seem right so I was wondering if

automatically resets the time spent

is ambiguous. But I think you're right, it's intended to work this way

@johnhope I assumed it should just set the total_time_spent to 0 initially as well, maybe we should change that behaviour as it does not really make sense to me either, but I am not sure about the number of affected users with this behaviour change

@johnhope it is documented so I guess it is intended,

Even if it is intended. I think this message is bit misleading. It should be something like "Hey, the value you want to subtract is bigger than the actual value of total_time_spent". According to me, it should not be a SUCCESSFUL message at least.

@acroitor @harry-hov I would not expect you to be able to subtract more time than exists.

If I spend 3h and then -4h, I am technically only subtracting 3h. Subtracting 4h from 0 is still 0.

The rule would basically be -- subtract the difference between time_spent. If time_spent will equal 0, then only subtract the actual time removed. This way it isn't confusing in the success messages as you can't ever subtract more time than time spent.

If I spend 3h and then -4h, I am technically only subtracting 3h. Subtracting 4h from 0 is still 0.

@gweaver If I spend 3h and then -4h. Value of total_time_spent remains unchanged i.e 3h. So technically we are subtracting nothing if time_spent exceeds total_time_spend?

@harry-hov this is what I would expect:

/spend 3h 

"total_time_spent": 10800
"human_total_time_spent": "3h"
"time_change": 10800
"human_time_change": "3h"

Message: "Added 3h time spent"

...

/spend 1h

"total_time_spent": 14400
"human_total_time_spent": "4h"
"time_change": 3600
"human_time_change": "1h"

Message: "Added 1h time spent"

...

/spend -5h

"total_time_spent": 0
"human_total_time_spent": "0"
"time_change": -14400
"human_time_change": "-4h"

Message: "Subtracted 4h spent time"

time_change and human_time_change should never be negative values that are greater than total_time_spent.

@gweaver I agree with the scenarios above. The only thing to mention(and I think you are aware of it) is that the last scenario where you subtract more than time spent, currently does not work as presented by you, which raises the question of how many customers are relying on that weird behaviour and can potentially be affected by the change in behaviour?

I still lean towards changing the behaviour, just because it makes sense, and I am not sure we have the numbers on customer usage on time spent feature, do we ?

@gweaver @acroitor Looks like we're all agreed it should work by setting the value to 0 instead of ignoring the negative time spent. Could we split the work to correct this behavior out from this issue to give @harry-hov an easier time?

I don't see too much duplication of work but fixing the behavior will require a docs update and we should check the impact as @acroitor says. @harry-hov could include the time_change and human_time_change attributes just as it works now, then in the next issue we fix the bug so the right amount of time is subtracted.

seems we will start tracking time estimates and time changes soon

• issue: #292824 (closed)
• mr: !55046 (merged)

oh its on mrs only I think

Works for me @johnhope. @acroitor in terms of changing the existing behavior, I'm not concerned about disrupting anyone honestly. It's a logical correction. In terms of usage, there are about 11.6k folks that track time and about 4.3k that set time estimates.

as long as we are ok with the behaviour change we can move forwards and do the fix and update the docs.

Issue for the bug fix is #322761 (closed)

added 1h of time spent at 2021-02-23

changed time estimate to 5h

removed time spent

added 1h of time spent at 2021-02-23

subtracted 1h of time spent at 2021-02-23

added 3h of time spent at 2021-02-23

subtracted 1h of time spent at 2021-02-23

assigned to @harry-hov

mentioned in merge request !55087 (merged)

[...] @harry-hov could include the time_change and human_time_change attributes just as it works now, [...]

Hey @johnhope @acroitor @gweaver, I just opened the Merge Request for this issue. Please take a look.

@harry-hov feel free to ping me on the MR when it is ready for a review.

@acroitor sure. Thanks Few tests failing. I'm on it.

mentioned in issue #322761 (closed)

marked this issue as related to #322761 (closed)

added workflowin dev label and removed workflowready for development label

added Community contribution label

changed milestone to %13.10

removed workflowin dev label

changed milestone to %13.11

added missed:13.10 label

changed milestone to %13.12

added missed:13.11 label

mentioned in merge request !61228 (closed)

changed milestone to %14.0

added missed:13.12 label

added workflowin review label

changed milestone to %14.1

added missed:14.0 label

changed milestone to %14.2

added missed:14.1 label

changed milestone to %14.3

added missed:14.2 label