Page MenuHomePhabricator

Conduit APIs to start and stop tracking time in phrequent
ClosedPublic

Authored by hach-que on Oct 16 2013, 12:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 5:12 PM
Unknown Object (File)
Fri, Dec 20, 11:39 AM
Unknown Object (File)
Thu, Dec 19, 11:04 AM
Unknown Object (File)
Thu, Dec 19, 12:31 AM
Unknown Object (File)
Sat, Dec 14, 4:21 PM
Unknown Object (File)
Fri, Dec 13, 11:36 AM
Unknown Object (File)
Thu, Dec 12, 7:54 PM
Unknown Object (File)
Thu, Dec 12, 7:14 PM

Details

Summary

This adds methods to start and stop tracking any arbitrary PHID in phrequent. Currently, this uses copy-pasted code from PhrequentTrackController. I had to do this because the code to start/stop was not abstracted into a common class.

Once the code to start/stop working is extracted into a re-usable class, the conduit API can use this as well.

Test Plan

I called the functions with a PHID of a task and ensured that the fields in the phrequent database table was being updated correctly.

Diff Detail

Branch
T3970
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

See T3569, D7349.

I think this is mostly good, but I want the CLI to be stack-aware. We probably need three commands:

  • arc tracking (or arc work or arc track or arc list-time or something): shows the stack
  • arc start (or whatever): pushes onto the stack
  • arc stop (or whatever): pops the stack

So usage would go like this:

$ arc tracking
(In Progress)   2h  T122: Fetch Lumber
(Suspended)    30m  T121: Build Cabin

This shows that you're currently fetching lumber. If something comes up, you might run:

$ arc start T123
Started: T123: Put out a fire

(In Progress)   1s  T123: Put out the fire
(Suspended)     2h  T122: Fetch Lumber
(Suspended)    30m  T121: Build Cabin

Once the fire is out, you'd run:

$ arc stop
Stopped: T123: Put out a fire 
Time Spent: 25m

(In Progress)   2h  T122: Fetch Lumber
(Suspended)    30m  T121: Build Cabin

That is, by default arc stop would just pop the stack. The full signature for arc stop should probably be:

$ arc stop [object | --all] [--note 'note'] [--at time]

Where --note lets you provide a note, and --at lets you provide an alternate (presumably, earlier) end time. --all would stop everything on your stack (e.g., if you're heading home for the day).

Do those seem reasonable? If we want to head in that direction, we should add query method, and stop shouldn't require a PHID (if one isn't provided, it should just pop the top of the stack). It should also probably return the object that was popped, and the time spent on it.

I don't think arc start T123 is a good syntax. If tomorrow i wish to add support for tracking time spent on wiki pages, or a pholio mock, then this might become confusing.

Instead, I feel arc start --task T123 is better, because it's explicit, and if a user really wishes to reduce keystrokes, they can just alias currently-being-forced-to-work-on="arc start --task" which is well supported in all shells (don't know about windows)

Shouldn't we allow a note for the "starting" point as well?

Most objects have short object names already (D123, M123, rExxxx, P123, L123, etc). These are unambiguous.

skyronic updated this revision to Unknown Object (????).Nov 8 2013, 3:07 PM

Added a phrequent tracking editor and renamed methods for phrequent conduit endpoints

Some minor convention/consistency stuff. Two inlines are things I think we should do in the future, but we don't need to do those for now.

src/applications/phrequent/conduit/ConduitAPI_phrequent_pop_Method.php
10 ↗(On Diff #16986)

pht() this

19 ↗(On Diff #16986)

Name this objectPHID to make the type more explicit/clear. (This is also more conventional.)

19 ↗(On Diff #16986)

I think this should be optional. If not present, whatever's on top of the stack is popped. We can do this in the future, though.

34 ↗(On Diff #16986)

Call this $object_phid -- many objects have both (global) PHIDs and (local) IDs.

39 ↗(On Diff #16986)

I think this should return the popped object, if one was popped. We can do that in the future, though.

src/applications/phrequent/conduit/ConduitAPI_phrequent_push_Method.php
10 ↗(On Diff #16986)

pht() and linewrap

19 ↗(On Diff #16986)

objectPHID

34–37 ↗(On Diff #16986)

phid, as above

src/applications/phrequent/editor/PhrequentTrackingEditor.php
3 ↗(On Diff #16986)

Add final.

5 ↗(On Diff #16986)

Typehint PhabricatorUser for $user.

This seems like a great idea... what happened to this review? Was this feature abandoned?

hach-que added a reviewer: skyronic.
hach-que edited the test plan for this revision. (Show Details)
hach-que edited edge metadata.
hach-que edited edge metadata.
  • Changes requested in code review
hach-que edited edge metadata.

Fix editor with more recent code

epriestley edited edge metadata.

Some notes inline but this seems close enough.

src/applications/phrequent/conduit/ConduitAPI_phrequent_Method.php
3–5

Remove these @group annotations, they're handled by config now.

src/applications/phrequent/conduit/ConduitAPI_phrequent_pop_Method.php
21–22 ↗(On Diff #23745)

In the future, it might be reasonable to let you adjust the start time from this call too. Maybe call timestamp something like endTime to allow for this and make usage more clear?

src/applications/phrequent/conduit/ConduitAPI_phrequent_push_Method.php
22 ↗(On Diff #23745)

Likewise, maybe call this startTime.

src/applications/phrequent/conduit/ConduitAPI_phrequent_tracking_Method.php
54 ↗(On Diff #23745)

Making this kind of stuff methods on the ranges is probably cleaner, at some point? Maybe getObjectTimeRanges() reuturns objects instead of a simple map or something.

73 ↗(On Diff #23745)

This should probably return modern pagination stuff, but as long as we fix it before building on top of it it's pretty whatever.

This revision is now accepted and ready to land.Jul 11 2014, 10:55 PM
hach-que edited edge metadata.
  • Changes requested in code review

Remove other @group annotations.

hach-que updated this revision to Diff 23778.

Closed by commit rP2101c3b6890d (authored by @hach-que).