Mailinglist Archive: yast-devel (233 mails)

< Previous Next >
[yast-devel] Re: [yast-commit] <rest-service> master : working eula backend, no access restrictions yet
  • From: Josef Reidinger <jreidinger@xxxxxxx>
  • Date: Thu, 8 Oct 2009 15:18:50 +0200
  • Message-id: <200910081518.51125.jreidinger@xxxxxxx>
Martin Kudlvasr write:
ref: refs/heads/master
commit f3713225650628c4b4ecaa247d9b7db1ea9f9f18
Author: Martin Kudlvasr <mkudlvasr@xxxxxxx>
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@xxxxxxxxxxxx
For additional commands, e-mail: yast-devel+help@xxxxxxxxxxxx

< Previous Next >
This Thread
  • No further messages