Acceptance between administrators #790

gmartincor · 2025-04-14T07:34:37Z

grabacion-de-pantalla-2025-04-14-a-las-92316_CkLB7uBF.1.mp4

Alliance Feature for Time Banks

This new feature that allows time banks to establish alliances with each other. This functionality extends the existing organization relationship model to enable time banks to collaborate more effectively.

The implementation includes:

A migration to create the organization_alliances table which tracks relationships between time banks.Only organization administrators can manage alliances.
New models and controllers to manage the alliance lifecycle (request, accept/reject, end alliance).
UI components integrated into the existing organizations views, allowing administrators to:
- Request alliances with other time banks
- Accept or reject incoming alliance requests
- View and manage their current alliances
- End active alliances when needed
Several tests to ensure the functionality works as expected.

Copilot

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

db/structure.sql: Language not supported

Copilot · 2025-04-14T07:35:31Z

config/locales/es.yml

+    actions: "Acciones"
+    sent: "Enviadas"
+    received: "Recibidas"
+    pending: "Pending"


The Spanish translation for 'pending' remains in English. Consider updating it (e.g., to 'Pendiente' or a more context-appropriate term) for consistency.

Suggested change

pending: "Pending"

pending: "Pendiente"

@gmartincor I think our Our colleague Copilot is right

Copilot · 2025-04-14T07:35:32Z

app/controllers/organization_alliances_controller.rb

+      target_organization_id: params[:organization_alliance][:target_organization_id],
+      status: "pending"


In the create action, the target_organization_id is accessed directly from params instead of using the alliance_params method. Consider refactoring to use strong parameters (alliance_params) to enhance consistency and security.

Suggested change

target_organization_id: params[:organization_alliance][:target_organization_id],

status: "pending"

target_organization_id: alliance_params[:target_organization_id],

@gmartincor another point for Copilot

franpb14 · 2025-04-23T15:49:58Z

Hi @gmartincor , I've been a bit busy lately and I continue busy, so I'll be slow in the PRs review, sorry for that 🙏.

Thanks for the contribution!

franpb14 · 2025-04-23T15:57:34Z

spec/models/organization_spec.rb

@@ -5,24 +5,20 @@
    it "validates content_type" do
      temp_file = Tempfile.new('test.txt')
      organization.logo.attach(io: File.open(temp_file.path), filename: 'test.txt')
-


revert this change since it only generates

franpb14 · 2025-04-23T15:57:41Z

spec/models/organization_spec.rb

      expect(organization).to be_invalid

      temp_file = Tempfile.new('test.svg')
      organization.logo.attach(io: File.open(temp_file.path), filename: 'test.svg')
-


franpb14 · 2025-04-23T15:57:47Z

spec/models/organization_spec.rb

      expect(organization).to be_invalid

      temp_file = Tempfile.new('test.png')
      organization.logo.attach(io: File.open(temp_file.path), filename: 'test.png')
-


franpb14 · 2025-04-23T15:57:52Z

spec/models/organization_spec.rb

      expect(organization).to be_valid
    end
  end

  describe '#display_id' do
    subject { organization.display_id }
-


franpb14 · 2025-04-23T16:03:46Z

remember that this means that you didn't let an empty space at the end of the file, it is a good practice to have it

franpb14 · 2025-04-23T16:19:37Z

app/controllers/organization_alliances_controller.rb

+  end
+
+  def create
+    @alliance = OrganizationAlliance.new(


there is no need for @ivar here I think

franpb14 · 2025-04-23T16:22:10Z

app/controllers/organization_alliances_controller.rb

+  end
+
+  def update
+    authorize @alliance


since we have before_action :authorize_admin we don't need an authorize here

franpb14 · 2025-04-23T16:22:18Z

app/controllers/organization_alliances_controller.rb

+  end
+
+  def destroy
+    authorize @alliance


franpb14 · 2025-04-23T16:25:03Z

app/controllers/organization_alliances_controller.rb

+    unless current_user.manages?(current_organization)
+      flash[:error] = t("organization_alliances.not_authorized")
+      redirect_to root_path
+    end


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

franpb14 · 2025-04-23T16:30:24Z

app/models/organization.rb

+  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


I'm not sure if this naming is clear enough, maybe:

initiated_alliances / received_alliances
outgoing_alliances / incoming_alliances

what do you think?

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 :)

franpb14 · 2025-04-23T20:09:43Z

app/models/organization.rb

+  def pending_sent_alliances
+    source_alliances.pending
+  end
+
+  def pending_received_alliances
+    target_alliances.pending
+  end


why not combine this with an or like the other 2?

I've implemented your suggestion to combine the methods for pending alliances using the or operator, just like we do with accepted and rejected alliances.

To make this change, I not only modified the Organization model by creating the pending_alliances method, but I also updated the controller organization_alliances_controller to use this new method:

# Before: current_organization.pending_sent_alliances.includes(:source_organization, :target_organization) + current_organization.pending_received_alliances.includes(:source_organization, :target_organization) # After: current_organization.pending_alliances.includes(:source_organization, :target_organization)

franpb14 · 2025-04-23T20:16:07Z

app/policies/organization_alliance_policy.rb

+    alliance = record
+    user.manages?(alliance.source_organization) || user.manages?(alliance.target_organization)


just to understand, why do we need this? and also this repeated code could be an auxiliar function

franpb14 · 2025-04-23T20:16:34Z

app/views/organization_alliances/index.html.erb

+<div class="row">
+  <div class="col-12 col-sm-12 col-md-12 col-lg-12">


col-12 is enough, since you are not giving other sizes

franpb14 · 2025-04-23T20:20:38Z

app/views/organization_alliances/index.html.erb

+                  <% if is_sender %>
+                    <%= t('organization_alliances.sent') %>
+                  <% else %>
+                    <%= t('organization_alliances.received') %>
+                  <% end %>


this could be just this:
<%= t("organization_alliances.#{is_sender ? 'sent' : 'received'}'") %>

franpb14 · 2025-04-23T20:23:35Z

app/views/organization_alliances/index.html.erb

+              <% is_sender = (alliance.source_organization_id == current_organization.id) %>
+              <% other_org = is_sender ? alliance.target_organization : alliance.source_organization %>


move this code to a helper, the views should have the less ruby code as possible

franpb14 · 2025-04-23T20:28:48Z

db/structure.sql

@@ -1,25 +1,13 @@
 SET statement_timeout = 0;
 SET lock_timeout = 0;
+SET idle_in_transaction_session_timeout = 0;


in this whole file there a bunch of changes not related with your migration and it could break, probably because the database version, make sure you only leave what's important and don't change anything else.

franpb14 · 2025-04-23T20:37:00Z

spec/controllers/organization_alliances_controller_spec.rb

+    let!(:pending_sent) {
+      OrganizationAlliance.create!(
+        source_organization: organization,
+        target_organization: other_organization,
+        status: "pending"
+      )
+    }
+
+    let!(:pending_received) {
+      OrganizationAlliance.create!(
+        source_organization: Fabricate(:organization),
+        target_organization: organization,
+        status: "pending"
+      )
+    }
+
+    let!(:accepted) {
+      OrganizationAlliance.create!(
+        source_organization: organization,
+        target_organization: Fabricate(:organization),
+        status: "accepted"
+      )
+    }
+
+    let!(:rejected) {
+      OrganizationAlliance.create!(
+        source_organization: organization,
+        target_organization: Fabricate(:organization),
+        status: "rejected"
+      )
+    }


use these let at the beggining, create a fabricator for this model and if the ! after let is not needed don't use it

franpb14 · 2025-04-23T20:38:22Z

spec/controllers/organization_alliances_controller_spec.rb

+    end
+
+    it "sets flash error if alliance cannot be created" do
+      # Try to create alliance with self which is invalid


do not use comments for that, it is already described in the test

franpb14 · 2025-04-23T20:39:10Z

spec/controllers/organization_alliances_controller_spec.rb

+    let!(:alliance) {
+      OrganizationAlliance.create!(
+        source_organization: Fabricate(:organization),
+        target_organization: organization,
+        status: "pending"
+      )
+    }


same move this to the beggining

franpb14 · 2025-04-23T20:39:49Z

spec/controllers/organization_alliances_controller_spec.rb

+    let!(:alliance) {
+      OrganizationAlliance.create!(
+        source_organization: organization,
+        target_organization: other_organization
+      )
+    }


same, you probably will be able to reuse some

franpb14 · 2025-04-23T20:41:07Z

spec/models/organization_alliance_spec.rb

+  let(:other_organization) { Fabricate(:organization) }
+
+  around do |example|
+    I18n.with_locale(:en) do


strange, I'm not sure about this

franpb14 · 2025-04-23T20:41:40Z

spec/models/organization_alliance_spec.rb

+      @pending_alliance = OrganizationAlliance.create!(
+        source_organization: organization,
+        target_organization: other_organization,
+        status: "pending"
+      )
+
+      @accepted_alliance = OrganizationAlliance.create!(
+        source_organization: Fabricate(:organization),
+        target_organization: Fabricate(:organization),
+        status: "accepted"
+      )
+
+      @rejected_alliance = OrganizationAlliance.create!(
+        source_organization: Fabricate(:organization),
+        target_organization: Fabricate(:organization),
+        status: "rejected"
+      )


these should be let at the beggining

franpb14 · 2025-04-23T20:43:05Z

spec/models/organization_spec.rb

+        alliance = OrganizationAlliance.create!(
+          source_organization: organization,
+          target_organization: other_organization
+        )


same use the fabricator at the beginning in this file

franpb14 · 2025-04-23T20:43:37Z

spec/models/organization_spec.rb

+        @pending_sent = OrganizationAlliance.create!(
+          source_organization: organization,
+          target_organization: other_organization,
+          status: "pending"
+        )
+
+        @pending_received = OrganizationAlliance.create!(
+          source_organization: third_organization,
+          target_organization: organization,
+          status: "pending"
+        )
+
+        @accepted_sent = OrganizationAlliance.create!(
+          source_organization: organization,
+          target_organization: Fabricate(:organization),
+          status: "accepted"
+        )
+
+        @accepted_received = OrganizationAlliance.create!(
+          source_organization: Fabricate(:organization),
+          target_organization: organization,
+          status: "accepted"
+        )
+
+        @rejected_sent = OrganizationAlliance.create!(
+          source_organization: organization,
+          target_organization: Fabricate(:organization),
+          status: "rejected"
+        )
+
+        @rejected_received = OrganizationAlliance.create!(
+          source_organization: Fabricate(:organization),
+          target_organization: organization,
+          status: "rejected"
+        )


franpb14 · 2025-04-23T20:46:35Z

I think that's all for now, I'd like to give it a second go when it's all fixed 😄

gmartincor added 4 commits April 13, 2025 15:41

alliance between diffents timebanks

88c808b

Bottom correction

838cbbe

class bottom corrected

3d63279

tests

f6616c9

gmartincor requested a review from Copilot April 14, 2025 07:34

Copilot AI reviewed Apr 14, 2025

View reviewed changes

gmartincor changed the base branch from develop to time-between-banks-base April 14, 2025 08:05

franpb14 reviewed Apr 23, 2025

View reviewed changes

gmartincor added a commit that referenced this pull request Apr 24, 2025

Correct comments #790

615eb44

gmartincor added 6 commits April 24, 2025 19:56

Refactor Code

c93d0a6

Correct comments #790

61888ac

structure.sql

560fdca

.

a60e50a

.

e14570c

.

13cd2c4

gmartincor force-pushed the acceptance-between-administrators branch from 2687a1a to 13cd2c4 Compare April 24, 2025 18:39

.

48d9950

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Acceptance between administrators #790

Acceptance between administrators #790

gmartincor commented Apr 14, 2025

Copilot AI left a comment

Copilot AI Apr 14, 2025

franpb14 Apr 23, 2025

Copilot AI Apr 14, 2025

franpb14 Apr 23, 2025

franpb14 commented Apr 23, 2025 •

edited

Loading

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 commented Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

gmartincor Apr 24, 2025

franpb14 Apr 23, 2025

gmartincor Apr 24, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025 •

edited

Loading

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025 •

edited

Loading

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 Apr 23, 2025

franpb14 commented Apr 23, 2025

		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],

		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

		alliance = record
		user.manages?(alliance.source_organization) \|\| user.manages?(alliance.target_organization)

		<div class="row">
		<div class="col-12 col-sm-12 col-md-12 col-lg-12">

		<% is_sender = (alliance.source_organization_id == current_organization.id) %>
		<% other_org = is_sender ? alliance.target_organization : alliance.source_organization %>

Acceptance between administrators #790

Are you sure you want to change the base?

Acceptance between administrators #790

Conversation

gmartincor commented Apr 14, 2025

Alliance Feature for Time Banks

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Apr 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI Apr 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franpb14 commented Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franpb14 commented Apr 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franpb14 Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franpb14 Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franpb14 commented Apr 23, 2025

franpb14 commented Apr 23, 2025 •

edited

Loading

franpb14 Apr 23, 2025 •

edited

Loading

franpb14 Apr 23, 2025 •

edited

Loading