Paths

Table of Contentst

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.

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

Lint

Lint Skipped

Unit

Tests Skipped

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

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
51	In modern code, you should be able to just `return $dialog;`.
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);
132–133	This should be `%d`, since the value is an integer.

This revision now requires changes to proceed.May 16 2014, 12:47 AM

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

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

Fixed trailing whitespace

Harbormaster completed remote builds in B506: Diff 21734.May 16 2014, 3:14 PM

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

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

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

Thanks for the contrib!

itsawesome

Revision Contents
Changeset List

Path

Size

src/

applications/

phrequent/

controller/

PhrequentTrackController.php

93 lines

Diff 21739

View Options

src/applications/phrequent/controller/PhrequentTrackController.php

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 timeClosedPublicActions