On 01/25, David Lutterkort wrote:
> On Wed, 2013-01-23 at 16:51 +0100, mfoj...@redhat.com wrote:
> > From: Michal Fojtik <mfoj...@redhat.com>
> >
> >
> > Signed-off-by: Michal fojtik <mfoj...@redhat.com>
>
> NAK because there's no test. Also a couple comments:
Yeah, will add tests in next revision, thanks for remind me that :)
>
> > ---
> > server/lib/cimi/models/base.rb | 11 ++++++++---
> > server/lib/cimi/models/schema.rb | 1 +
> > 2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/server/lib/cimi/models/base.rb b/server/lib/cimi/models/base.rb
> > index 5fdb69a..1393ae4 100644
> > --- a/server/lib/cimi/models/base.rb
> > +++ b/server/lib/cimi/models/base.rb
> > @@ -170,8 +170,12 @@ class CIMI::Model::Resource
> > # Prepare to serialize
> > def prepare
> > self.class.schema.collections.map { |coll| coll.name }.each do |n|
> > - self[n].href = "#{self.id}/#{n}" unless self[n].href
> > - self[n].id = "#{self.id}/#{n}" if !self[n].entries.empty?
> > + if !@filter_attrs.empty? and !@filter_attrs.include?(n)
> > + @attribute_values[n] = nil
>
> @attribute_values[n] is the same as self[n], and we should stick to one
> way of writing that.
I have tried to use this approach in first place but I got this error:
E, [2013-01-28T11:17:47.721669 #4436] ERROR -- 500: [RuntimeError]
Collection Class::MachineVolumeCollection must have one of id and href set
I think it is because you can't directly overide the @attribute_values and
if you use []() method then 'self.class.schema.convert(a, v)' is used that
perform this validation here.
I modified the [] method to delete the Hash key when you try to assign a
nil value.
>
> > + else
> > + self[n].href = "#{self.id}/#{n}" if !self[n].href
> > + self[n].id = "#{self.id}/#{n}" if !self[n].entries.empty?
> > + end
>
> This if block is basically saying 'if we are not selecting collection n,
> set it to nil, otherwise set id/href to sth useful'
>
> First off, we should rename everything called 'filter' in the code to
> 'select', since that's what it's used for (I had to do a bit of grepping
> to realize all the filter stuff is only used for handling $select)
>
> Second, the branches in the condition should be reversed to avoid the
> '!' so that we can rewrite this as
>
> if @selected.include?(n)
> self[n].href = "#{self.id}/#{n}" if !self[n].href
> self[n].id = "#{self.id}/#{n}" if !self[n].entries.empty?
> else
> self[n] = nil
> end
ACK. I changed the method in this way. Thanks for pointing this out. For
some reason my brain use negation instead of reverse :)
> > diff --git a/server/lib/cimi/models/schema.rb
> > b/server/lib/cimi/models/schema.rb
> > index 4acd2dc..24a5a0e 100644
> > --- a/server/lib/cimi/models/schema.rb
> > +++ b/server/lib/cimi/models/schema.rb
> > @@ -241,6 +241,7 @@ class CIMI::Model::Schema
> > end
> >
> > def to_xml(model, xml)
> > + return if model[name].nil?
> > model[name].prepare
> > if model[name].entries.empty?
> > xml[xml_name] = { "href" => model[name].href }
>
> The same thing needs to happen to to_json, no ?
Thanks! Fixed.
>
> David
>
--
Michal Fojtik <mfoj...@redhat.com>
Deltacloud API, CloudForms
