On 05/06/2011 11:02 AM, Jan Provaznik wrote:
Hi Tomas, sending some notes inline.
On 05/04/2011 06:09 PM, tsedovic(a)redhat.com wrote:
> From: Tomas Sedovic<tsedovic(a)redhat.com>
>
> This implements Redmine #1348.
> ---
> src/app/models/user.rb | 1 +
> src/app/models/view_state.rb | 31 +++++++++++
> .../migrate/20110504124706_create_view_states.rb | 16 ++++++
> src/spec/factories/view_state.rb | 7 +++
> src/spec/models/view_state_spec.rb | 57 ++++++++++++++++++++
> 5 files changed, 112 insertions(+), 0 deletions(-)
> create mode 100644 src/app/models/view_state.rb
> create mode 100644 src/db/migrate/20110504124706_create_view_states.rb
> create mode 100644 src/spec/factories/view_state.rb
> create mode 100644 src/spec/models/view_state_spec.rb
>
> diff --git a/src/app/models/user.rb b/src/app/models/user.rb
> index ead459a..f795542 100644
> --- a/src/app/models/user.rb
> +++ b/src/app/models/user.rb
> @@ -59,6 +59,7 @@ class User< ActiveRecord::Base
>
> has_many :permissions
> has_many :owned_instances, :class_name => "Instance", :foreign_key
=>
> "owner_id"
> + has_many :view_states
>
> belongs_to :quota, :autosave => true
> accepts_nested_attributes_for :quota
> diff --git a/src/app/models/view_state.rb b/src/app/models/view_state.rb
> new file mode 100644
> index 0000000..de0a516
> --- /dev/null
> +++ b/src/app/models/view_state.rb
> @@ -0,0 +1,31 @@
> +# == Schema Information
> +# Schema version: 20110502094458
> +#
> +# Table name: view_states
> +#
> +# id :integer not null, primary key
> +# created_at :datetime
> +# updated_at :datetime
> +#
> +
> +class ViewState< ActiveRecord::Base
> + belongs_to :user
> +
> + validates_presence_of :name
> + validates_uniqueness_of :name
> + validates_presence_of :controller
> + validates_presence_of :action
user should be required for each state and I would make
key_value_attributes non-optional:
validates_presence_of :user_id
validates_presence_of :key_value_attributes
> +
> + # Things that make sense to remember for the temporary ViewState but
> not for
> + # the persistent state -- such as position in the pagination.
> + SESSION_ONLY_KEYS = ['page']
> +
> + def attributes
def key_value_attributes
I think it makes sense in this case redefine directly
key_value_attributes getter/setter - we will always use hash as
input/output for this attr and it enforces correct format/encoding of
this attr (there will never be some invalid string but only valid JSON)
> + JSON.load(key_value_attributes) unless key_value_attributes.blank?
> + end
> +
> + def attributes=(attrs)
> + persistent_attributes = attrs.reject { |k,v|
> SESSION_ONLY_KEYS.include? k }
> + self.key_value_attributes = JSON.dump(persistent_attributes)
> + end
> +end
> diff --git a/src/db/migrate/20110504124706_create_view_states.rb
> b/src/db/migrate/20110504124706_create_view_states.rb
> new file mode 100644
> index 0000000..14acb3e
> --- /dev/null
> +++ b/src/db/migrate/20110504124706_create_view_states.rb
> @@ -0,0 +1,16 @@
> +class CreateViewStates< ActiveRecord::Migration
> + def self.up
> + create_table :view_states do |t|
> + t.string :name, :null => false
> + t.string :controller, :null => false
> + t.string :action, :null => false
> + t.text :key_value_attributes
Nit: personally I thi it's better to use some singular name for this
attribute, for example "state", convention is that plurals represent
array of values (for example has_many association), but this will surely
work too if you want to keep this.
> +
> + t.timestamps
Missing :user_id attr
> + end
> + end
> +
> + def self.down
> + drop_table :view_states
> + end
> +end
> diff --git a/src/spec/factories/view_state.rb
> b/src/spec/factories/view_state.rb
> new file mode 100644
> index 0000000..9760ff1
> --- /dev/null
> +++ b/src/spec/factories/view_state.rb
> @@ -0,0 +1,7 @@
> +Factory.define :view_state do |vs|
> + vs.sequence(:name) { |n| "view-state#{n}" }
> + vs.controller 'pools'
> + vs.action 'view'
> + vs.key_value_attributes '{"sort-column": "name",
"sort-order":
> "desc", "columns": ["name", "deployments",
"instances"]}'
> + vs.association :user
> +end
> diff --git a/src/spec/models/view_state_spec.rb
> b/src/spec/models/view_state_spec.rb
> new file mode 100644
> index 0000000..5fd13f0
> --- /dev/null
> +++ b/src/spec/models/view_state_spec.rb
> @@ -0,0 +1,57 @@
> +require 'spec_helper'
> +
> +describe ViewState do
> +
> + before(:each) do
> + @view_state = Factory :view_state
> + end
> +
> + it "should require a unique name" do
> + vs = Factory.build :view_state, :name => nil
> + vs.should_not be_valid
> +
> + vs.name = @view_state.name
> + vs.should_not be_valid
> +
> + vs.name = "a nice unique name"
> + vs.should be_valid
> + end
> +
> + it "should require a controller and action" do
> + @view_state.controller = nil
> + @view_state.should_not be_valid
> +
> + @view_state.controller = "deployments"
> + @view_state.should be_valid
> +
> + @view_state.action = nil
> + @view_state.should_not be_valid
> +
> + @view_state.action = "view"
> + @view_state.should be_valid
> + end
> +
> + it "should provide an access to the key-value attributes" do
> + @view_state.attributes.should eql({'sort-column' => 'name',
> 'sort-order' => 'desc',
> + 'columns' => ['name', 'deployments',
'instances']})
> + end
> +
> + it "should allow setting the key-value attributes" do
> + custom_attributes = {'a' => 1, 'b' => 2, 'nested'
=> {'x' =>
> 'hello', 'y' => 'world'} }
> + @view_state.attributes = custom_attributes
> + @view_state.save!
> +
> + @view_state.attributes.should eql(custom_attributes)
> + ViewState.find((a)view_state).attributes.should eql(custom_attributes)
> + end
> +
> + it "should not save attributes that are only relevant in a user
> session" do
> + @view_state.attributes = { 'sort-column' => 'name',
'page' => 3 }
> + @view_state.save!
> +
> + expected_attributes = {'sort-column' => 'name'}
> + @view_state.attributes.should eql(expected_attributes)
> + ViewState.find((a)view_state).attributes.should eql(expected_attributes)
> + end
> +
> +end
Thanks Jan, all great points.
I'll fix them and resend soon.
Thomas
--
No trees were killed to send this message, but a large number of
electrons were terribly inconvenienced.