Move Dashboard and Panel edit history out of the way
ClosedPublic
Actions

Authored by lpriestley on May 24 2014, 4:18 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T5170: Move Dashboard and Panel edit history out of the way

Commits

Restricted Diffusion Commit
rP7de4e80907c7: Move Dashboard and Panel edit history out of the way

Summary

Fixes T5170, Create new page for dashboard history

Test Plan

Open dashboard, manage dashboard, click on "View History". Dashboard history should appear. Panel history should appear on panel view page under panel.

Diff Detail

Repository

rP Phabricator

Branch

dashboardhistorypage

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 690
Build 690: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

lpriestley updated this revision to Diff 22040.May 24 2014, 4:18 PM

lpriestley retitled this revision from to Move Dashboard and Panel edit history out of the way.

lpriestley updated this object.

lpriestley edited the test plan for this revision. (Show Details)

lpriestley added a reviewer: epriestley.

lpriestley added a task: T5170: Move Dashboard and Panel edit history out of the way.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 24 2014, 4:18 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B686: Diff 22040.May 24 2014, 4:19 PM

Couple of minor tweaks, looks great otherwise.

I particularly enjoy the delicate sophistication added to the timelines by the addition of the end caps. They really bring the page together.

src/applications/dashboard/controller/PhabricatorDashboardHistoryController.php
23	We don't need panels in this controller, right? Removing this (so we don't load data we don't need) will make the page load a little faster.
61–62	(This is my fault from earlier, but we don't actually use this, and can just remove these lines.)
src/applications/dashboard/controller/PhabricatorDashboardManageController.php
117–118	I think you can just remove these, users don't need edit permission to view history.

This revision now requires changes to proceed.May 24 2014, 4:21 PM

lpriestley added inline comments.May 24 2014, 7:26 PM

src/applications/dashboard/controller/PhabricatorDashboardHistoryController.php
61–62	Can you clarify why this isn't needed?

Addressing code review

Harbormaster completed remote builds in B690: Diff 22044.May 24 2014, 7:28 PM

epriestley added inline comments.May 24 2014, 7:28 PM

src/applications/dashboard/controller/PhabricatorDashboardHistoryController.php
61–62	The variable `$engine` is never used, so constructing this object has no effect. Older versions of the rendering code required you to construct an engine and pass it to the `TransactionView`. However, at some point it became clear that almost all of these engines were just boilerplate, so we removed the requirement. If you don't pass an engine in, `TransactionView` will just make on on its own. When I wrote the original Dashboard controller, I presumably copy/pasted this chunk of code from some older code which still builds an engine. I think I removed the unnecessary `setEngine($engine)` call but neglected to remove the construction of the `$engine` variable.