Martin Kudlvasr write:

ref: refs/heads/master commit f3713225650628c4b4ecaa247d9b7db1ea9f9f18 Author: Martin Kudlvasr <mkudlvasr@suse.cz> 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: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org