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

[Peter Mottram]

Due to a change made back in 2008...

commit fd989774c974cd0869384c25c0f6bb866377c3ad
Author: Ruslan Zakirov <***@bestpractical.com>
Date: Tue Oct 21 07:43:38 2008 +0000

* add possible_columns method to Jifty/Action/Record*


... 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.

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

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?

R.
PeteM

[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.

Post by Peter Mottram
... 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.

Post by Peter Mottram
I've come up with a very simple change which pulls my lovely virtual

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.

Post by Peter Mottram
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