From fdb31203d3433bf8c295e050da1f5ebc39460370 Mon Sep 17 00:00:00 2001
From: Marc Anguera Insa <srmarc.ai@gmail.com>
Date: Sat, 12 May 2018 23:11:53 +0200
Subject: [PATCH 1/2] minor organizations code/tests clean up

---
 app/controllers/organizations_controller.rb | 19 +++---
 app/models/organization.rb                  | 12 +---
 app/views/organizations/_form.html.erb      |  1 +
 app/views/organizations/edit.html.erb       |  7 ---
 app/views/organizations/index.html.erb      |  2 +-
 spec/models/organization_spec.rb            | 66 ++++++++++++---------
 6 files changed, 49 insertions(+), 58 deletions(-)

diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb
index 1d84b947d..a08ac9e25 100644
--- a/app/controllers/organizations_controller.rb
+++ b/app/controllers/organizations_controller.rb
@@ -1,20 +1,12 @@
 class OrganizationsController < ApplicationController
-  before_filter :load_resource
-
-  def load_resource
-    if params[:id]
-      @organization = Organization.find(params[:id])
-    else
-      @organizations = Organization.all
-    end
-  end
+  before_filter :load_resource, except: [:new, :index, :create]
 
   def new
     @organization = Organization.new
   end
 
   def index
-    @organizations = @organizations.matching(params[:q]) if params[:q].present?
+    @organizations = Organization.all
   end
 
   def show
@@ -27,7 +19,8 @@ def show
   end
 
   def create
-    @organization = @organizations.build organization_params
+    @organization = Organization.new(organization_params)
+
     if @organization.save
       redirect_to @organization, status: :created
     else
@@ -57,6 +50,10 @@ def set_current
 
   private
 
+  def load_resource
+    @organization = Organization.find(params[:id])
+  end
+
   def organization_params
     params[:organization].permit(*%w[name theme email phone web
                                      public_opening_times description address
diff --git a/app/models/organization.rb b/app/models/organization.rb
index 47d81c789..9f664d9b6 100644
--- a/app/models/organization.rb
+++ b/app/models/organization.rb
@@ -2,11 +2,7 @@ class Organization < ActiveRecord::Base
   has_many :members, dependent: :destroy
   has_many :users, -> { order "members.created_at DESC" }, through: :members
   has_many :all_accounts, class_name: "Account", inverse_of: :organization
-  has_many :all_movements, class_name: "Movement", through: :all_accounts, source: :movements do
-    def with_transfer
-      joins(transfer: :movements)
-    end
-  end
+  has_many :all_movements, class_name: "Movement", through: :all_accounts, source: :movements
   has_many :all_transfers, class_name: "Transfer", through: :all_movements, source: :transfer
   has_one :account, as: :accountable
   has_many :member_accounts, through: :members, source: :account
@@ -15,11 +11,7 @@ def with_transfer
   has_many :inquiries
   has_many :documents, as: :documentable
 
-  scope :matching, ->(str) {
-    where(Organization.arel_table[:name].matches("%#{str}%"))
-  }
-
-  validates :name, uniqueness: true
+  validates :name, presence: true, uniqueness: true
 
   before_validation :ensure_url
   after_create :create_account
diff --git a/app/views/organizations/_form.html.erb b/app/views/organizations/_form.html.erb
index 47d4af54e..56bfbbb17 100644
--- a/app/views/organizations/_form.html.erb
+++ b/app/views/organizations/_form.html.erb
@@ -1,3 +1,4 @@
+<%= show_error_messages!(@organization) %>
 <%= simple_form_for @organization do |f| %>
   <%= f.input :name %>
   <%= f.input :email %>
diff --git a/app/views/organizations/edit.html.erb b/app/views/organizations/edit.html.erb
index 958a4182e..49919e276 100644
--- a/app/views/organizations/edit.html.erb
+++ b/app/views/organizations/edit.html.erb
@@ -1,10 +1,3 @@
-<% if @organization.errors.present? %>
-  <% @organization.errors.full_messages.each do |msg| %>
-    <div class="alert alert-danger" role="alert">
-      <%= msg %>
-    </div>
-  <% end %>
-<% end %>
 <h1>
   <%= link_to @organization, organization_path(@organization) %>
   <small><%= t 'global.edit' %></small>
diff --git a/app/views/organizations/index.html.erb b/app/views/organizations/index.html.erb
index 896027ab0..11c0c2fc6 100644
--- a/app/views/organizations/index.html.erb
+++ b/app/views/organizations/index.html.erb
@@ -2,7 +2,7 @@
   <%= Organization.model_name.human(count: :many) %>
 </h1>
 
-<ul class="nav nav-pills">
+<ul class="nav navbar-nav pull-right">
   <% if superadmin? %>
     <li>
       <%= link_to new_organization_path do %>
diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb
index c8af92b96..5eb95a903 100644
--- a/spec/models/organization_spec.rb
+++ b/spec/models/organization_spec.rb
@@ -17,35 +17,43 @@
     end
   end
 
-  it "1:  without http & https" do
-    organization.web = "www.casa.com"
-    expect(organization).to be_valid
-    expect(organization.web).to eq "http://www.casa.com"
-  end
-  it "2: with http" do
-    organization.web = "http://www.casa.com"
-    expect(organization).to be_valid
-    expect(organization.web).to eq "http://www.casa.com"
-  end
-  it "3: with https" do
-    organization.web = "https://www.casa.com"
-    expect(organization).to be_valid
-    expect(organization.web).to eq "https://www.casa.com"
-  end
-  it "4: blank" do
-    organization.web = ""
-    expect(organization).to be_valid
-    expect(organization.web).to eq ""
-  end
-  it "5: nil" do
-    organization.web = ""
-    expect(organization).to be_valid
-    expect(organization.web).to eq ""
+  describe 'ensure_url validation' do
+    it "without http & https" do
+      organization.web = "www.casa.com"
+      expect(organization).to be_valid
+      expect(organization.web).to eq "http://www.casa.com"
+    end
+    it "with http" do
+      organization.web = "http://www.casa.com"
+      expect(organization).to be_valid
+      expect(organization.web).to eq "http://www.casa.com"
+    end
+    it "with https" do
+      organization.web = "https://www.casa.com"
+      expect(organization).to be_valid
+      expect(organization.web).to eq "https://www.casa.com"
+    end
+    it "with blank value" do
+      organization.web = ""
+      expect(organization).to be_valid
+      expect(organization.web).to eq ""
+    end
+    it "with nil value" do
+      organization.web = nil
+      expect(organization).to be_valid
+      expect(organization.web).to eq nil
+    end
+    it "with an invalid" do
+      organization.web = "la casa"
+      expect(organization).not_to be_valid
+      expect(organization.web).to eq "la casa"
+      expect(organization.errors.size).to eq 1
+    end
   end
-  it "6: no url" do
-    organization.web = "la casa"
-    expect(organization).not_to be_valid
-    expect(organization.web).to eq "la casa"
-    expect(organization.errors.size).to eq 1
+
+  it 'name is mandatory' do
+    organization.name = nil
+    organization.save
+    expect(organization.errors[:name]).to include(I18n.t('errors.messages.blank'))
   end
 end

From b9d9b9636c806d48aad7e75cd353160572750689 Mon Sep 17 00:00:00 2001
From: Marc Anguera Insa <srmarc.ai@gmail.com>
Date: Mon, 14 May 2018 21:45:52 +0200
Subject: [PATCH 2/2] organizations: add constraints at db level: not nullable
 name, unique-index name

---
 app/controllers/organizations_controller.rb           |  2 +-
 ...514193153_change_organizations_name_constraints.rb | 11 +++++++++++
 db/schema.rb                                          |  6 ++++--
 spec/models/organization_spec.rb                      |  5 +++++
 4 files changed, 21 insertions(+), 3 deletions(-)
 create mode 100644 db/migrate/20180514193153_change_organizations_name_constraints.rb

diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb
index a08ac9e25..0a394f172 100644
--- a/app/controllers/organizations_controller.rb
+++ b/app/controllers/organizations_controller.rb
@@ -1,5 +1,5 @@
 class OrganizationsController < ApplicationController
-  before_filter :load_resource, except: [:new, :index, :create]
+  before_filter :load_resource, only: [:show, :edit, :update, :destroy]
 
   def new
     @organization = Organization.new
diff --git a/db/migrate/20180514193153_change_organizations_name_constraints.rb b/db/migrate/20180514193153_change_organizations_name_constraints.rb
new file mode 100644
index 000000000..afc7642cc
--- /dev/null
+++ b/db/migrate/20180514193153_change_organizations_name_constraints.rb
@@ -0,0 +1,11 @@
+class ChangeOrganizationsNameConstraints < ActiveRecord::Migration
+  def up
+    change_column_null :organizations, :name, false
+    add_index :organizations, :name, unique: true
+  end
+
+  def down
+    change_column_null :organizations, :name, true
+    remove_index :organizations, :name
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index e4e2e2e1d..a9dc3fb7d 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180221161343) do
+ActiveRecord::Schema.define(version: 20180514193153) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -102,7 +102,7 @@
   add_index "movements", ["transfer_id"], name: "index_movements_on_transfer_id", using: :btree
 
   create_table "organizations", force: :cascade do |t|
-    t.string   "name"
+    t.string   "name",                 limit: 255, null: false
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer  "reg_number_seq"
@@ -118,6 +118,8 @@
     t.string   "domain"
   end
 
+  add_index "organizations", ["name"], name: "index_organizations_on_name", unique: true, using: :btree
+
   create_table "posts", force: :cascade do |t|
     t.string   "title"
     t.string   "type"
diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb
index 5eb95a903..10e3db473 100644
--- a/spec/models/organization_spec.rb
+++ b/spec/models/organization_spec.rb
@@ -23,26 +23,31 @@
       expect(organization).to be_valid
       expect(organization.web).to eq "http://www.casa.com"
     end
+
     it "with http" do
       organization.web = "http://www.casa.com"
       expect(organization).to be_valid
       expect(organization.web).to eq "http://www.casa.com"
     end
+
     it "with https" do
       organization.web = "https://www.casa.com"
       expect(organization).to be_valid
       expect(organization.web).to eq "https://www.casa.com"
     end
+
     it "with blank value" do
       organization.web = ""
       expect(organization).to be_valid
       expect(organization.web).to eq ""
     end
+
     it "with nil value" do
       organization.web = nil
       expect(organization).to be_valid
       expect(organization.web).to eq nil
     end
+
     it "with an invalid" do
       organization.web = "la casa"
       expect(organization).not_to be_valid