Martin Kudlvasr write:
> ref: refs/heads/master
> commit f3713225650628c4b4ecaa247d9b7db1ea9f9f18
> Author: Martin Kudlvasr <[email protected]>
> Date: Thu Oct 8 13:52:39 2009 +0200
>
> working eula backend, no access restrictions yet
> ---
> plugins/eulas/app/controllers/eulas_controller.rb | 5 +-
> plugins/eulas/app/models/license.rb | 56
> +++++++++++++-------- plugins/eulas/app/views/eulas/index.html.erb |
> 2 +-
> plugins/eulas/app/views/eulas/show.html.erb | 5 ++-
> plugins/eulas/test/unit/license_test.rb | 20 +++----
> 5 files changed, 51 insertions(+), 37 deletions(-)
>
> diff --git a/plugins/eulas/app/controllers/eulas_controller.rb
> b/plugins/eulas/app/controllers/eulas_controller.rb index 83b0fd5..a449bee
> 100644
> --- a/plugins/eulas/app/controllers/eulas_controller.rb
> +++ b/plugins/eulas/app/controllers/eulas_controller.rb
> @@ -1,6 +1,6 @@
> # = Eula controller
> # Serves licences and handles notices about acceptations.
> -class EulaController < ApplicationController
> +class EulasController < ApplicationController
>
> before_filter :login_required
>
> @@ -14,7 +14,8 @@ class EulaController < ApplicationController
> end
>
> def show
> - @license = License.find params[:id]
> + @id = params[:id].to_i
I suggest check if params[:id] exist (for usage via curl e.g.) and if is not
present throw exception. Now it cause unknown method without any user friendly
message what goes wrong.
> + @license = License.find @id
> if not params[:lang].nil? then @license.load_text params[:lang] end
^^^
you use @license variable even if License.find can return nil, so another easy
to crash part
hint 'if not' == 'unless'
hint2 use ruby one liner, no C oneliner
so it should look like
raise <some_exception> unless params[:id]
@id = params[:id].to_i
@license = License.find @id
raise <some_exception> unless @license
@license.load_text params[:lang] if params[:lang]
> logger.debug @license.inspect
> respond_to do |format|
> diff --git a/plugins/eulas/app/models/license.rb
> b/plugins/eulas/app/models/license.rb index cab752f..6024eb8 100644
> --- a/plugins/eulas/app/models/license.rb
> +++ b/plugins/eulas/app/models/license.rb
> @@ -10,9 +10,8 @@ class License
> # :text_lang - language, of the current eula translation
> # :only_show - some licenses only need to be show and user doesn't have
> to change the radio button to "accept" # :accepted - whether this license
> was already accepted
> - # :last - true if this is the last unaccepted license. Set by
> controller.
>
I suggest to you to try generate documentation for your code (rake doc:app).
This doesn't work. Look e.g. on time module how to document attributes.
> - attr_accessor :name, :langs_hash, :langs_list, :accepted, :text,
> :text_lang, :only_show, :last + attr_accessor :name, :langs_hash,
> :langs_list, :accepted, :text, :text_lang, :only_show
>
> RESOURCES_DIR =
> File.join(File.dirname(__FILE__),"..","..","config","resources") VAR_DIR
> = File.join(File.dirname(__FILE__),"..","..","var") @@ -55,32 +54,31
^^^ see discussion about place for var directory on yast-devel. It should
point to /var/lib/yastws/...
also resources maybe should go to /usr/share/yastws
> @@ class License
> raise CorruptedFileException.new license_dir
> end
> @name = name
> - # @last is true if this is the last license to be accepted.
> - # this is to be set by class method
> - @last = false
> end
>
> - def self.find(name)
> - license = new name
> - # lets be sure, that @text and @text_lang is never nil
> - license.load_text "en"
> - license
> + def self.find(id)
> + name = license_names[id-1] # let ids in find start from 1
> + if name.nil? then
> + nil
> + else
> + license = new name
> + # lets be sure, that @text and @text_lang is never nil
> + license.load_text "en"
> + license
> + end
> end
>
> - def self.find_all
> + def self.license_names
> config = YaST::ConfigFile.new(:eula)
> - license_names = config["licenses"]
> - license_names.collect{|ln| new ln}
> + begin
> + config["licenses"]
> + rescue Exception => e
> + raise CorruptedFileException.new config.path
> + end
> end
>
> - def self.next
> - all = find_all
> - unaccepted_count = all.inject(0){|sum,license| sum + (license.accepted
> ? 0 : 1)} - next_license = all.find{|license| not license.accepted}
> - if not next_license.nil? then
> - next_license.last = unaccepted_count == 1
> - end
> - next_license
> + def self.find_all
> + Licenses.new license_names.collect{|ln| new ln}
> end
>
> def save
> @@ -133,12 +131,12 @@ class License
> xml.name @name
> xml.accepted (@accepted, {:type => "boolean"})
> xml.last (@last, {:type => "boolean"})
> - xml.textlang @text_lang
> xml.availablelangs({:type => "array"}) do
> @langs_list.each do |lang|
> xml.availablelang lang
> end
> end
> + xml.textlang @text_lang
> xml.text @text
> end
> end
> @@ -149,3 +147,17 @@ class License
> end
>
> end
> +
> +class Licenses < Array
> + def to_xml( options = {} )
> + xml = options[:builder] ||= Builder::XmlMarkup.new(options)
> + xml.instruct! unless options[:skip_instruct]
> + xml.eulas(:type => :array) do
> + each {|eula| eula.to_xml(:builder => xml, :skip_instruct => true)}
> + end
> + end
> +
> + def to_json
> + Hash.from_xml(to_xml).to_json
> + end
> +end
> diff --git a/plugins/eulas/app/views/eulas/index.html.erb
> b/plugins/eulas/app/views/eulas/index.html.erb index 646e8fb..5a82692
> 100644
> --- a/plugins/eulas/app/views/eulas/index.html.erb
> +++ b/plugins/eulas/app/views/eulas/index.html.erb
> @@ -4,7 +4,7 @@
> <% @licenses.each_with_index do |license,index| -%>
> <tr>
> <td>
> - <%= link_to license.name, :only_path => :true, :controller => "eulas",
> :action => :show, :id => index -%> + <%= link_to license.name,
> :only_path => :true, :controller => "eulas", :action => :show, :id =>
> (index+1) -%> </td><td>
> <%= license.accepted ? "v" : "x" -%>
> </td>
> diff --git a/plugins/eulas/app/views/eulas/show.html.erb
> b/plugins/eulas/app/views/eulas/show.html.erb index 8a01c61..f0e2ffe
> 100644
> --- a/plugins/eulas/app/views/eulas/show.html.erb
> +++ b/plugins/eulas/app/views/eulas/show.html.erb
> @@ -10,10 +10,13 @@
> </tr>
> <tr>
> <td>Available translations:</td>
> - <td><%= @license.langs_list.join(", ") -%></td>
> + <td><%= @license.langs_list.collect{ |lang| link_to lang, :only_path =>
> :true, :controller => "eulas", :action => :show, :id => @id, :params =>
> {:lang => lang} }.join(", ") -%></td> </tr>
> <tr>
> <td>Current translation:</td>
> <td><%= @license.text_lang -%></td>
> </tr>
> </table>
> +<pre>
> +<%= @license.text -%>
> +</pre>
> diff --git a/plugins/eulas/test/unit/license_test.rb
> b/plugins/eulas/test/unit/license_test.rb index 8f4b895..6b3f437 100644
> --- a/plugins/eulas/test/unit/license_test.rb
> +++ b/plugins/eulas/test/unit/license_test.rb
> @@ -2,20 +2,21 @@ require File.expand_path(File.dirname(__FILE__) +
> "/../test_helper")
>
> class LicenseTest < ActiveSupport::TestCase
> def test_create
> - license = License.find 'openSUSE-11.1'
> + license = License.find 1
> assert_not_nil license
> assert_equal( license.accepted, false)
> assert_equal( license.name, 'openSUSE-11.1' )
> end
>
> def test_accepted
> - license = License.find 'SLED-10-SP3'
> + license = License.find 2
> assert_not_nil license
> assert_equal( license.accepted, true)
> + assert_equal( license.name, 'SLED-10-SP3')
> end
>
> def test_load_text
> - license = License.find 'openSUSE-11.1'
> + license = License.find 1
> license.load_text 'de'
> assert_not_nil license.text
> assert_equal( license.text_lang, 'de')
> @@ -25,13 +26,13 @@ class LicenseTest < ActiveSupport::TestCase
> end
>
> def test_to_xml
> - license = License.find 'openSUSE-11.1'
> + license = License.find 1
> license.load_text 'de'
> assert_not_nil license.to_xml
> end
>
> def test_to_json
> - license = License.find 'openSUSE-11.1'
> + license = License.find 1
> license.load_text 'de'
> assert_not_nil license.to_json
> end
> @@ -40,12 +41,9 @@ class LicenseTest < ActiveSupport::TestCase
> licenses = License.find_all
> assert_equal( licenses.length, 2)
> assert_equal(licenses[1].name, 'SLED-10-SP3')
> - assert_nil licenses[1].text
> + assert_nil licenses[1].text
> + assert_not_nil licenses.to_xml
> + assert_not_nil licenses.to_json
> end
>
> - def test_next
> - license = License.next
> - assert_equal(license.name, 'openSUSE-11.1')
> - assert_false license.accepted
> - end
> end
>
At general I suggest you to use more visible separating of public and private
part of model as it makes your code more readability. At least you should mark
methods as private or as public. (I suggest to put at top of code public
methods and to bottom after private keyword private methods, because reader
start from top of code.)
Josef
--
Josef Reidinger
YaST team
maintainer of perl-Bootloader, YaST2-Repair, webyast modules language and
time
--
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
