Page MenuHomePhabricator
Differential D9147

Added a popup to both start and end times with Phrequent allowing the user to edit their start/end times. Also added a field for notes on the stop time
ClosedPublic
Actions

Authored by bwinterton on May 15 2014, 11:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 18, 6:15 AM
Unknown Object (File)
Fri, Dec 13, 1:14 AM
Unknown Object (File)
Fri, Dec 6, 8:37 AM
Unknown Object (File)
Tue, Dec 3, 9:27 PM
Unknown Object (File)
Fri, Nov 29, 1:59 AM
Unknown Object (File)
Tue, Nov 26, 1:51 AM
Unknown Object (File)
Tue, Nov 26, 1:51 AM
Unknown Object (File)
Mon, Nov 25, 8:48 PM
View All Files
Subscribers
chad
epriestley
Korvin
Tokens
"Yellow Medal" token, awarded by chad.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rP715bf1fd55c7: Added a popup to both start and end times with Phrequent allowing the user to…
Summary

I have added a dialog box which pops up when a user starts or stops tracking time on an issue with Phrequent. These dialogs allow the user to modify the time if it so happens that they forgot to either clock in or out.
I have also added a Note field in the dialog when a user stops tracking time. This allows them to enter a note about the time, and is entered into the database, but is currently (as far as I know) not visible anywhere in Phabricator.
I have made these changes according to the suggestions found in T3568

Also, upon clocking in or out, if the time entered is a future time, an error is returned and the user is asked to enter a valid time.

Test Plan

Start tracking time and edit the start date/time, then end the time and edit that timestamp as well.
Also, try entering future dates/times and ensure that the dialog reports an error and asks for the time again.
Ensure that these edited times are recorded properly.

Diff Detail

Repository
rP Phabricator
Branch
T3568
Lint
Lint Passed
SeverityLocationCodeMessage
Auto-Fixsrc/applications/phrequent/controller/PhrequentTrackController.php:56TXT6Trailing Whitespace
Unit
No Test Coverage
Build Status
Buildable 505
Build 505: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

bwinterton updated this revision to Diff 21716.May 15 2014, 11:38 PM
bwinterton added a task: Restricted Maniphest Task.
bwinterton retitled this revision from to Added a popup to both start and end times with Phrequent allowing the user to edit their start/end times. Also added a field for notes on the stop time.
bwinterton updated this object.
bwinterton edited the test plan for this revision. (Show Details)
bwinterton added a reviewer: epriestley.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 15 2014, 11:38 PM
Herald added subscribers: Korvin, epriestley. · View Herald Transcript
Harbormaster completed remote builds in B498: Diff 21716.May 15 2014, 11:39 PM
epriestley requested changes to this revision.May 16 2014, 12:47 AM
epriestley edited edge metadata.

This mostly looks good. Some notes inline. The biggest issue here is untranslatable strings. When you do things like this:

pht('%s Tracking Time', $verb);

...it's impossible to translate the text: the translation may depend on the verb, and the verb can't be translated in any case. Instead, you generally need to break strings out so they appear fully, not as fragments:

switch ($verb) {
  case 'stop':
    $button_text = pht('Stop Tracking Time');
    $title_text = pht('...');
    // ... etc ...
    break;
  case 'start':
    $button_text = pht('Start Tracking Time');
    $title_text = pht('...');
    // ... etc ...
    break;
}

These strings can be translated. (We should have better documentation about this, but haven't really started getting super serious about translations yet.)

src/applications/phrequent/controller/PhrequentTrackController.php
58–59

In modern code, you can do this slightly more simply with:

$dialog = $this->newDialog();

...as long as you're in a Controller.

69–75

Use $epoch_control->setError($err) to set red error text next to the control.

Use a pattern like this to render a standard dialog error:

$errors = array();
// ...
$errors[] = pht('The date you have entered ...');
// ...
$dialog->setErrors($errors);
77

In modern code, you should be able to just return $dialog;.

133–134

This should be %d, since the value is an integer.

This revision now requires changes to proceed.May 16 2014, 12:47 AM
bwinterton added a comment.May 16 2014, 2:20 PM

Thanks for your help! I will get those changes made right away.

bwinterton updated this revision to Diff 21733.May 16 2014, 3:11 PM
bwinterton edited edge metadata.
  • Made changes requested by epriestley and added checking for invalid dates
Harbormaster completed remote builds in B505: Diff 21733.May 16 2014, 3:12 PM
bwinterton updated this revision to Diff 21734.May 16 2014, 3:13 PM
bwinterton edited edge metadata.
  • Fixed trailing whitespace
Harbormaster completed remote builds in B506: Diff 21734.May 16 2014, 3:14 PM
epriestley accepted this revision.May 16 2014, 3:51 PM
epriestley edited edge metadata.

There's a tiny little string issue still but this looks good to me. I'll just fix the string thing in the pull. Thanks!

src/applications/phrequent/controller/PhrequentTrackController.php
34

This one is still untranslatable.

This revision is now accepted and ready to land.May 16 2014, 3:51 PM
bwinterton added a comment.May 16 2014, 3:52 PM

I am sorry, I completely missed that one. $verb_formatted can also be removed. Sorry that I did not catch that.

epriestley closed this revision.May 16 2014, 3:54 PM
epriestley updated this revision to Diff 21739.

Closed by commit rP715bf1fd55c7 (authored by @bwinterton, committed by @epriestley).

chad awarded a token.May 16 2014, 3:56 PM
chad added a subscriber: chad.

Thanks for the contrib!

itsawesome

Revision Contents
Changeset List

PathSize
src/
applications/
phrequent/
controller/
94 lines

Diff 21733

View Options

src/applications/phrequent/controller/PhrequentTrackController.php

Loading...
Phabricator · Built by Phacility