[jifty-devel] Jifty::Collection virtual columns in models - a proposed patch

[Thomas Sibley]

Hi Peter,

Thanks for the detailed mail and tracking down the actual commit that
caused problems for you.  I've included comments inline below.

On 09/27/2011 12:53 PM, Peter Mottram wrote:
> ... which I've been meaning to hunt down for some time but only now had
> a big enough itch, we ended up losing automatic rendering of virtual
> columns which refer to Jifty::Collections. In the past couple of years
> I've got along by adding params in MyApp::Action::CreateFoo to include
> the virtual column in CRUD views but today I got tired of that and
> started searching for the culprit which ended up being the above change.

A more Jifty-like workaround is to define your own possible_columns in
MyApp::Action or MyApp::Action::Record::Create/Update.  All of your CRUD
actions should already inherit from those classes, and you'll only have
to implement the virtual workaround in one place in your app.

> I've come up with a very simple change which pulls my lovely virtual
> collection columns back into my CRUD views:

I'm not convinced that the arbitrary "anything that refers to a
collection" condition makes much sense.  You really don't want virtual
columns of any kind in your CUD views since you can't CUD them.  It's a
deficiency in the CRUD views that they use readonly Update actions for
record display.  Any patch for trunk that makes virtual columns visible
in CRUD should probably be in CRUD itself.

> diff --git a/lib/Jifty/Action/Record.pm b/lib/Jifty/Action/Record.pm
> index cc5b6a2..b33fc8f 100644
> --- a/lib/Jifty/Action/Record.pm
> +++ b/lib/Jifty/Action/Record.pm
> @@ -536,6 +536,7 @@ sub possible_columns {
>      my $self = shift;
>      return grep {
>          $_->container
> +            or UNIVERSAL::isa( $_->refers_to, 'Jifty::Collection' )
>              or ( !$_->private and !$_->virtual and !$_->computed and
> $_->type n
>      } $self->record->columns;
>  }
> 
> The full Jifty test suite still passes with this change and I cannot see
> anything that it should break. Any chance this (or something similar)
> might make it into trunk?

It will break anything that's relying on the documentation (explicitly
not virtual columns) right above this method being correct.  The Jifty
test suite is great, but it doesn't cover everything and there are
complex apps out there which might break due to such an API change.

Cheers,
Thomas