Correct comments #790

Author: gmartincor

app/controllers/organization_alliances_controller.rb

@@ -9,8 +9,7 @@ def index
 
     @alliances = case @status
                  when "pending"
-                   current_organization.pending_sent_alliances.includes(:source_organization, :target_organization) +
-                   current_organization.pending_received_alliances.includes(:source_organization, :target_organization)
+                   current_organization.pending_alliances.includes(:source_organization, :target_organization)
                  when "accepted"
                    current_organization.accepted_alliances.includes(:source_organization, :target_organization)
                  when "rejected"
@@ -21,24 +20,22 @@ def index
   end
 
   def create
-    @alliance = OrganizationAlliance.new(
+    alliance = OrganizationAlliance.new(
       source_organization: current_organization,
-      target_organization_id: params[:organization_alliance][:target_organization_id],
+      target_organization_id: alliance_params[:target_organization_id],
       status: "pending"
     )
 
-    if @alliance.save
+    if alliance.save
       flash[:notice] = t("organization_alliances.created")
     else
-      flash[:error] = @alliance.errors.full_messages.to_sentence
+      flash[:error] = alliance.errors.full_messages.to_sentence
     end
 
     redirect_back fallback_location: organizations_path
   end
 
   def update
-    authorize @alliance
-
     if @alliance.update(status: params[:status])
       flash[:notice] = t("organization_alliances.updated")
     else
@@ -49,8 +46,6 @@ def update
   end
 
   def destroy
-    authorize @alliance
-
     if @alliance.destroy
       flash[:notice] = t("organization_alliances.destroyed")
     else
@@ -67,10 +62,10 @@ def find_alliance
   end
 
   def authorize_admin
-    unless current_user.manages?(current_organization)
-      flash[:error] = t("organization_alliances.not_authorized")
-      redirect_to root_path
-    end
+    return if current_user.manages?(current_organization)
+
+    flash[:error] = t("organization_alliances.not_authorized")
+    redirect_to root_path
   end
 
   def alliance_params

app/helpers/organizations_helper.rb

@@ -14,4 +14,12 @@ def allied_organizations
     organizations = Organization.where(id: allied_org_ids + [current_organization.id])
     organizations.order(:name)
   end
+
+  def alliance_initiator?(alliance)
+    alliance.source_organization_id == current_organization.id
+  end
+
+  def alliance_recipient(alliance)
+    alliance_initiator?(alliance) ? alliance.target_organization : alliance.source_organization
+  end
 end

app/models/organization.rb

@@ -24,8 +24,8 @@ class Organization < ApplicationRecord
   has_many :inquiries
   has_many :documents, as: :documentable, dependent: :destroy
   has_many :petitions, dependent: :delete_all
-  has_many :source_alliances, class_name: "OrganizationAlliance", foreign_key: "source_organization_id", dependent: :destroy
-  has_many :target_alliances, class_name: "OrganizationAlliance", foreign_key: "target_organization_id", dependent: :destroy
+  has_many :initiated_alliances, class_name: "OrganizationAlliance", foreign_key: "source_organization_id", dependent: :destroy
+  has_many :received_alliances, class_name: "OrganizationAlliance", foreign_key: "target_organization_id", dependent: :destroy
 
   validates :name, presence: true, uniqueness: true
 
@@ -54,39 +54,30 @@ def display_name_with_uid
     self
   end
 
-  # Returns the id to be displayed in the :new transfer page with the given
-  # destination_accountable
-  #
-  # @params destination_accountable [Organization | Object] target of a transfer
-  # @return [Integer | String]
   def display_id
     account.accountable_id
   end
 
   def alliance_with(organization)
-    source_alliances.find_by(target_organization: organization) ||
-      target_alliances.find_by(source_organization: organization)
+    initiated_alliances.find_by(target_organization: organization) ||
+      received_alliances.find_by(source_organization: organization)
   end
 
-  def pending_sent_alliances
-    source_alliances.pending
-  end
-
-  def pending_received_alliances
-    target_alliances.pending
+  def pending_alliances
+    initiated_alliances.pending.or(received_alliances.pending)
   end
 
   def accepted_alliances
-    source_alliances.accepted.or(target_alliances.accepted)
+    initiated_alliances.accepted.or(received_alliances.accepted)
   end
 
   def rejected_alliances
-    source_alliances.rejected.or(target_alliances.rejected)
+    initiated_alliances.rejected.or(received_alliances.rejected)
   end
 
   def allied_organizations
-    source_org_ids = source_alliances.accepted.pluck(:target_organization_id)
-    target_org_ids = target_alliances.accepted.pluck(:source_organization_id)
+    source_org_ids = initiated_alliances.accepted.pluck(:target_organization_id)
+    target_org_ids = received_alliances.accepted.pluck(:source_organization_id)
     Organization.where(id: source_org_ids + target_org_ids)
   end
 

app/policies/organization_alliance_policy.rb

@@ -49,22 +49,17 @@
           </thead>
           <tbody>
             <% @alliances.each do |alliance| %>
-              <% is_sender = (alliance.source_organization_id == current_organization.id) %>
-              <% other_org = is_sender ? alliance.target_organization : alliance.source_organization %>
+              <% other_org = alliance_recipient(alliance) %>
               <tr>
                 <td><%= link_to other_org.name, other_org %></td>
                 <td><%= other_org.city %></td>
                 <td><%= other_org.members.count %></td>
                 <td>
-                  <% if is_sender %>
-                    <%= t('organization_alliances.sent') %>
-                  <% else %>
-                    <%= t('organization_alliances.received') %>
-                  <% end %>
+                  <%= t("organization_alliances.#{alliance_initiator?(alliance) ? 'sent' : 'received'}") %>
                 </td>
                 <% if @status == 'pending' %>
                   <td>
-                    <% if is_sender %>
+                    <% if alliance_initiator?(alliance) %>
                       <%= link_to t('organization_alliances.cancel_request'),
                           organization_alliance_path(alliance),
                           method: :delete,
@@ -99,4 +94,4 @@
       </div>
     </div>
   </div>
-</div>
+</div>

app/views/organization_alliances/index.html.erb

@@ -13,4 +13,4 @@
   <% elsif alliance.rejected? %>
     <span class="badge rounded-pill bg-danger"><%= t('organization_alliances.rejected') %></span>
   <% end %>
-<% end %>
+<% end %>

app/views/organizations/_alliance_button.html.erb

@@ -12,4 +12,4 @@
       <%= render "organizations/alliance_button", organization: org %>
     </td>
   <% end %>
-</tr>
+</tr>

app/views/organizations/_organizations_row.html.erb

@@ -40,4 +40,4 @@
       <%= paginate @organizations %>
     </div>
   </div>
-</div>
+</div>

app/views/organizations/index.html.erb

@@ -621,4 +621,4 @@ en:
       last: Last
       next: Next
       previous: Previous
-      truncate: Truncate
+      truncate: Truncate

config/locales/en.yml

@@ -386,7 +386,7 @@ es:
     actions: "Acciones"
     sent: "Enviadas"
     received: "Recibidas"
-    pending: "Pending"
+    pending: "Pendientes"
     active: "Activa"
     rejected: "Rechazada"
     request_alliance: "Solicitar alianza"
@@ -621,4 +621,4 @@ es:
       last: Ultima »
       next: Siguiente ›
       previous: "‹ Anterior"
-      truncate: "…"
+      truncate: "…"

config/locales/es.yml

@@ -136,14 +136,11 @@
         )
       end
 
-      it "returns pending sent alliances" do
-        expect(organization.pending_sent_alliances).to include(@pending_sent)
-        expect(organization.pending_sent_alliances).not_to include(@pending_received)
-      end
-
-      it "returns pending received alliances" do
-        expect(organization.pending_received_alliances).to include(@pending_received)
-        expect(organization.pending_received_alliances).not_to include(@pending_sent)
+      it "returns pending alliances" do
+        expect(organization.pending_alliances).to include(@pending_sent, @pending_received)
+        expect(organization.pending_alliances).not_to include(
+          @accepted_sent, @accepted_received, @rejected_sent, @rejected_received
+        )
       end
 
       it "returns accepted alliances" do