Thanks for the review! I'm about to send out a revised patchset that address your
concerns - all except the fact that the logs page isn't actually linked to anywhere
from the UI. I don't think it's been fully decided where this page will appear
yet; however, my understanding is that this feature is going to be heavily iterated prior
to the next release, so there's a desire to get some base functionality out there so
others can start commenting and suggesting improvements.
Mainn
----- Original Message -----
From: "Jan Provaznik" <jprovazn(a)redhat.com>
To: "Tzu-Mainn Chen" <tzumainn(a)redhat.com>
Cc: aeolus-devel(a)lists.fedorahosted.org
Sent: Monday, May 7, 2012 8:54:19 AM
Subject: Re: [PATCH 0/5 conductor] (Rev 2) RM 3145 - create a basic event history page
On 05/03/2012 04:52 PM, Tzu-Mainn Chen wrote:
> This patchset creates a logs/ page that displays event history. It
> should work even without soft deletion implemented; although once
> soft deletiion is implemented, the changes will automatically be
> reflected.
>
> This revision of the patchset implements many suggestions from
> matty_dubs and jprovazn, including:
>
> - increasing the readability of the filter code in load_logs
> - updating the haml to match current standards
> - permission checks
> - dealing with legacy data
> - many minor stylistic fixes
>
> It also includes:
>
> - additional filters by state
> - mouseover for additional details
>
> Mainn
>
Hi Mainn,
the code works quite well. Have couple additional questions:
The logs page is not linked from anywhere in UI, users have to
specify
URI to get on this page - is this intention?
There was a thread on this list about keeping ruby conventions
(especially >80 lines). I think it was after you sent your patch
first
time, though if you are going to do some updates/rebase, line
splitting
would be appreciated.
It would be nice to have at least basic tests for this new feature,
nothing big, just make sure that if a user goes to logs/index page,
events are listed and that basic filtering works.
Jan