Page MenuHomePhabricator

Settings History
ClosedPublic

Authored by fabe on Dec 30 2014, 4:18 PM.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T6545: Show unified config edit log in Config web UI
Commits
Restricted Diffusion Commit
rP86eb7c0ec446: Settings History
Summary

Shows a timeline of all modified settings Fixes T6545
Will show all settings (no pagination, should be not so difficult to add if needed but most installs won't have hundreds of settings changes)
I'm not happy by how the PhabricatorConfigTransaction object is instructed to render the config keys but i don't see any other reasonable way.
We could always show the keys though.

Test Plan

Changed settings and called the history page

Diff Detail

Repository
rP Phabricator
Branch
configaudit
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 3391
Build 3398: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

fabe retitled this revision from to Settings History.
fabe updated this object.
fabe edited the test plan for this revision. (Show Details)
epriestley added a reviewer: epriestley.

This looks great.

I think we can make the "displayKeys" part a little less hacky by doing this:

  • Add a setRenderAsFeed() method to PhabricatorApplicationTransactionView.
  • If the flag is set, in PhabricatorApplicationTransactionView->renderEvent(), call getTitleForFeed() instead of getTitle().
    • You'll have to pass a $story, but I think you can just create a dummy object. As far as I can tell, no callsites actually use this parameter, and I can't think of a reason why any would. We can likely remove it at some point.
  • Then, implement getTitleForFeed() in PhabricatorConfigTransaction. You can look at other Transaction classes for an example, but basically it's a copy of getTitle() that just renders alincoln closed T123 instead of alincoln closed this task, similar to what you've done already.

Basically the same thing as this diff, but that approach would be a bit more general and should work for any kind of object.

This revision now requires changes to proceed.Dec 31 2014, 3:57 PM
fabe edited edge metadata.

Remove all the hacks from last year ;)
calling getTitleForFeed without a Parameter is still bad and might break other use cases.
However making up a dummy $story is tricky as it expects a PhabricatorFeedStoryData Object which is a DAO...

Ok, i've checked all the implementations of getTitleForFeed and getApplicationTransactionTitleForFeed which get $story passed and it is never used anywhere...
Want me to submit a diff removing it?

Yeah, let's just remove it if it's too hard to build a dummy object.

The rest of this looks perfect.

epriestley edited edge metadata.

I'll just land this and we can do the removal in a followup, I think it's OK to have this slightly-skethcy = null stuff for a little while.

This revision is now accepted and ready to land.Jan 1 2015, 2:51 PM
This revision was automatically updated to reflect the committed changes.