-
Notifications
You must be signed in to change notification settings - Fork 75
Acceptance between administrators #790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: time-between-banks-base
Are you sure you want to change the base?
Changes from 4 commits
88c808b
838cbbe
3d63279
f6616c9
c93d0a6
61888ac
560fdca
a60e50a
e14570c
13cd2c4
48d9950
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,79 @@ | ||||||||
| class OrganizationAlliancesController < ApplicationController | ||||||||
| before_action :authenticate_user! | ||||||||
| before_action :member_should_exist_and_be_active | ||||||||
| before_action :authorize_admin | ||||||||
| before_action :find_alliance, only: [:update, :destroy] | ||||||||
|
|
||||||||
| def index | ||||||||
| @status = params[:status] || "pending" | ||||||||
|
|
||||||||
| @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) | ||||||||
| when "accepted" | ||||||||
| current_organization.accepted_alliances.includes(:source_organization, :target_organization) | ||||||||
| when "rejected" | ||||||||
| current_organization.rejected_alliances.includes(:source_organization, :target_organization) | ||||||||
| else | ||||||||
| [] | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| def create | ||||||||
| @alliance = OrganizationAlliance.new( | ||||||||
| source_organization: current_organization, | ||||||||
| target_organization_id: params[:organization_alliance][:target_organization_id], | ||||||||
| status: "pending" | ||||||||
|
||||||||
| target_organization_id: params[:organization_alliance][:target_organization_id], | |
| status: "pending" | |
| target_organization_id: alliance_params[:target_organization_id], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmartincor another point for Copilot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we have before_action :authorize_admin we don't need an authorize here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code could be prettier using a guard clause:
if current_user.manages?(current_organization) return
flash[:error] = t("organization_alliances.not_authorized")
redirect_to root_pat
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this naming is clear enough, maybe: initiated_alliances / received_alliances what do you think?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I´m agree with you, I have changed source_alliances by source_alliances and target_alliances by received_alliances. Thank you very much for your advice @franpb14 :) |
||
|
|
||
| validates :name, presence: true, uniqueness: true | ||
|
|
||
|
|
@@ -61,6 +63,27 @@ 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) | ||
| end | ||
|
|
||
| def pending_sent_alliances | ||
| source_alliances.pending | ||
| end | ||
|
|
||
| def pending_received_alliances | ||
| target_alliances.pending | ||
| end | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not combine this with an
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've implemented your suggestion to combine the methods for pending alliances using the To make this change, I not only modified the |
||
|
|
||
| def accepted_alliances | ||
| source_alliances.accepted.or(target_alliances.accepted) | ||
| end | ||
|
|
||
| def rejected_alliances | ||
| source_alliances.rejected.or(target_alliances.rejected) | ||
| end | ||
|
|
||
| def ensure_reg_number_seq! | ||
| update_column(:reg_number_seq, members.maximum(:member_uid)) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| class OrganizationAlliance < ApplicationRecord | ||
| belongs_to :source_organization, class_name: "Organization" | ||
| belongs_to :target_organization, class_name: "Organization" | ||
|
|
||
| enum status: { pending: 0, accepted: 1, rejected: 2 } | ||
|
|
||
| validates :source_organization_id, presence: true | ||
| validates :target_organization_id, presence: true | ||
| validates :target_organization_id, uniqueness: { scope: :source_organization_id } | ||
| validate :cannot_ally_with_self | ||
|
|
||
| scope :pending, -> { where(status: "pending") } | ||
| scope :accepted, -> { where(status: "accepted") } | ||
| scope :rejected, -> { where(status: "rejected") } | ||
|
|
||
| private | ||
|
|
||
| def cannot_ally_with_self | ||
| if source_organization_id == target_organization_id | ||
| errors.add(:base, "Cannot create an alliance with yourself") | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| class OrganizationAlliancePolicy < ApplicationPolicy | ||
| def update? | ||
| alliance = record | ||
| user.manages?(alliance.source_organization) || user.manages?(alliance.target_organization) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to understand, why do we need this? and also this repeated code could be an auxiliar function |
||
| end | ||
|
|
||
| def destroy? | ||
| alliance = record | ||
| user.manages?(alliance.source_organization) || user.manages?(alliance.target_organization) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| <h1><%= t('organization_alliances.title') %></h1> | ||
|
|
||
| <div class="row"> | ||
| <div class="col-12 col-sm-12 col-md-12 col-lg-12"> | ||
|
Comment on lines
+3
to
+4
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. col-12 is enough, since you are not giving other sizes |
||
| <ul class="nav nav-pills actions-menu"> | ||
| <li class="nav-item"> | ||
| <%= link_to organization_alliances_path(status: 'pending'), class: "nav-link #{'active' if @status == 'pending'}" do %> | ||
| <%= glyph :time %> | ||
| <%= t('organization_alliances.status.pending') %> | ||
| <% end %> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <%= link_to organization_alliances_path(status: 'accepted'), class: "nav-link #{'active' if @status == 'accepted'}" do %> | ||
| <%= glyph :ok %> | ||
| <%= t('organization_alliances.status.accepted') %> | ||
| <% end %> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <%= link_to organization_alliances_path(status: 'rejected'), class: "nav-link #{'active' if @status == 'rejected'}" do %> | ||
| <%= glyph :remove %> | ||
| <%= t('organization_alliances.status.rejected') %> | ||
| <% end %> | ||
| </li> | ||
| <li class="nav-item ms-auto"> | ||
| <%= link_to organizations_path, class: "text-primary" do %> | ||
| <%= glyph :search %> | ||
| <%= t('organization_alliances.search_organizations') %> | ||
| <% end %> | ||
| </li> | ||
| </ul> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="row"> | ||
| <div class="col-md-12"> | ||
| <div class="card"> | ||
| <div class="card-body table-responsive"> | ||
| <table class="table table-hover table-sm"> | ||
| <thead> | ||
| <tr> | ||
| <th><%= t('organization_alliances.organization') %></th> | ||
| <th><%= t('organization_alliances.city') %></th> | ||
| <th><%= t('organization_alliances.members') %></th> | ||
| <th><%= t('organization_alliances.type') %></th> | ||
| <% if @status != 'rejected' %> | ||
| <th><%= t('organization_alliances.actions') %></th> | ||
| <% end %> | ||
| </tr> | ||
| </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 %> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move this code to a helper, the views should have the less ruby code as possible |
||
| <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 %> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could be just this: |
||
| </td> | ||
| <% if @status == 'pending' %> | ||
| <td> | ||
| <% if is_sender %> | ||
| <%= link_to t('organization_alliances.cancel_request'), | ||
| organization_alliance_path(alliance), | ||
| method: :delete, | ||
| class: 'btn btn-danger', | ||
| data: { confirm: t('organization_alliances.confirm_cancel') } %> | ||
| <% else %> | ||
| <div> | ||
| <%= link_to t('organization_alliances.accept'), | ||
| organization_alliance_path(alliance, status: 'accepted'), | ||
| method: :put, | ||
| class: 'btn btn-primary' %> | ||
| <%= link_to t('organization_alliances.reject'), | ||
| organization_alliance_path(alliance, status: 'rejected'), | ||
| method: :put, | ||
| class: 'btn btn-danger' %> | ||
| </div> | ||
| <% end %> | ||
| </td> | ||
| <% elsif @status == 'accepted' %> | ||
| <td> | ||
| <%= link_to t('organization_alliances.end_alliance'), | ||
| organization_alliance_path(alliance), | ||
| method: :delete, | ||
| class: 'btn btn-danger', | ||
| data: { confirm: t('organization_alliances.confirm_end') } %> | ||
| </td> | ||
| <% end %> | ||
| </tr> | ||
| <% end %> | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| <% if current_user&.manages?(current_organization) && organization != current_organization %> | ||
| <% alliance = current_organization.alliance_with(organization) %> | ||
| <% if alliance.nil? %> | ||
| <%= link_to t('organization_alliances.request_alliance'), | ||
| organization_alliances_path(organization_alliance: { target_organization_id: organization.id }), | ||
| method: :post, | ||
| class: 'btn btn-secondary', | ||
| aria: { label: t('organization_alliances.request_alliance_for', org: organization.name) } %> | ||
| <% elsif alliance.pending? %> | ||
| <span class="badge rounded-pill bg-secondary"><%= t('organization_alliances.pending') %></span> | ||
| <% elsif alliance.accepted? %> | ||
| <span class="badge rounded-pill bg-primary"><%= t('organization_alliances.active') %></span> | ||
| <% elsif alliance.rejected? %> | ||
| <span class="badge rounded-pill bg-danger"><%= t('organization_alliances.rejected') %></span> | ||
| <% end %> | ||
| <% end %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no need for
@ivar here I think