From 11048ece08636035225d8723b4c1a0c5cc531b64 Mon Sep 17 00:00:00 2001 From: franpb14 Date: Sat, 15 May 2021 12:24:17 +0200 Subject: [PATCH 1/4] feat: time can now be spent on an offer from another organization To do this, 3 transfers are made: member1 --> organization1; organization1 --> organization2; organization2 --> member2. In order to reuse the implemented code and not make it unnecessarily long, it was necessary to make some modifications to the implemented code. Issue #638. --- app/controllers/offers_controller.rb | 2 +- app/controllers/transfers_controller.rb | 40 +++++++++++++++++++------ app/models/organization.rb | 4 +++ app/models/transfer_factory.rb | 12 ++++++-- app/views/offers/show.html.erb | 11 +++++++ app/views/organizations/show.html.erb | 5 ++++ config/locales/en.yml | 1 + 7 files changed, 62 insertions(+), 13 deletions(-) diff --git a/app/controllers/offers_controller.rb b/app/controllers/offers_controller.rb index 232ba52e1..7dfcad407 100644 --- a/app/controllers/offers_controller.rb +++ b/app/controllers/offers_controller.rb @@ -6,7 +6,7 @@ def model def show super - member = @offer.user.members.find_by(organization: current_organization) + member = @offer.user.members.find_by(organization: @offer.organization) @destination_account = member.account if member end end diff --git a/app/controllers/transfers_controller.rb b/app/controllers/transfers_controller.rb index e65506566..f67ad3be0 100644 --- a/app/controllers/transfers_controller.rb +++ b/app/controllers/transfers_controller.rb @@ -5,16 +5,12 @@ def create @source = find_source @account = Account.find(transfer_params[:destination]) - transfer = Transfer.new( - transfer_params.merge(source: @source, destination: @account) - ) - - persister = ::Persister::TransferPersister.new(transfer) + create_persisters - if persister.save + if persisters_saved? redirect_to redirect_target else - redirect_back fallback_location: redirect_target, alert: transfer.errors.full_messages.to_sentence + redirect_back fallback_location: redirect_target, alert: @transfer.errors.full_messages.to_sentence end end @@ -23,7 +19,8 @@ def new current_organization, current_user, params[:offer], - params[:destination_account_id] + params[:destination_account_id], + params[:organization_id] || current_organization.id ) render( @@ -57,12 +54,37 @@ def find_source end end + def create_persisters + source_organization = @source.organization.account + destination_organization = @account.organization.account + + if source_organization == destination_organization + @persister = transfer_persister_between(@source, @account) + else + @pers_src_org = transfer_persister_between(@source, source_organization) + @pers_between_orgs = transfer_persister_between(source_organization, destination_organization) + @pers_org_acc = transfer_persister_between(destination_organization, @account) + end + end + + def transfer_persister_between(source, destination) + @transfer = Transfer.new( + transfer_params.merge(source: source, destination: destination) + ) + + ::Persister::TransferPersister.new(@transfer) + end + + def persisters_saved? + @persister&.save || @pers_src_org&.save && @pers_between_orgs&.save && @pers_org_acc&.save + end + def redirect_target case accountable = @account.accountable when Organization accountable when Member - accountable.user + accountable.organization == current_organization ? accountable.user : @source.accountable.user else raise ArgumentError end diff --git a/app/models/organization.rb b/app/models/organization.rb index dfc7a4b00..d0111fab0 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -65,6 +65,10 @@ def next_reg_number_seq reg_number_seq end + def global_balance + Account.where(organization_id: id).sum(:balance) + end + def ensure_url return if web.blank? || URI.parse(web).is_a?(URI::HTTP) rescue diff --git a/app/models/transfer_factory.rb b/app/models/transfer_factory.rb index a093299cd..fd4235a89 100644 --- a/app/models/transfer_factory.rb +++ b/app/models/transfer_factory.rb @@ -1,16 +1,22 @@ class TransferFactory - def initialize(current_organization, current_user, offer_id, destination_account_id) + def initialize(current_organization, current_user, offer_id, destination_account_id, + destination_organization_id) @current_organization = current_organization @current_user = current_user @offer_id = offer_id @destination_account_id = destination_account_id + @destination_organization_id = destination_organization_id.to_i + end + + def destination_organization + Organization.find(@destination_organization_id) end # Returns the offer that is the subject of the transfer # # @return [Maybe] def offer - current_organization.offers.find_by_id(offer_id) + destination_organization.offers.find_by_id(offer_id) end # Returns a new instance of Transfer with the data provided @@ -73,7 +79,7 @@ def admin? # # @return [Account] def destination_account - @destination_account ||= current_organization + @destination_account ||= destination_organization .all_accounts .find(destination_account_id) end diff --git a/app/views/offers/show.html.erb b/app/views/offers/show.html.erb index a3afc8fa3..33f3b6087 100644 --- a/app/views/offers/show.html.erb +++ b/app/views/offers/show.html.erb @@ -11,5 +11,16 @@ <% end %> <% end %>

+<% else %> +

+ <% if current_user and @offer.user != current_user %> + <%= link_to new_transfer_path(id: @offer.user.id, offer: @offer.id, destination_account_id: @destination_account.id, + organization_id: @offer.organization.id), + class: "btn btn-success" do %> + <%= glyph :time %> + <%= t ".give_time_for" %> + <% end %> + <% end %> +

<% end %> <%= render "shared/post", post: @offer %> diff --git a/app/views/organizations/show.html.erb b/app/views/organizations/show.html.erb index fb5eaee7c..940ab4a07 100644 --- a/app/views/organizations/show.html.erb +++ b/app/views/organizations/show.html.erb @@ -78,6 +78,11 @@ <%= t 'global.balance' %> <%= seconds_to_hm(@organization.account.try(:balance)) %> +
+ + <%= t 'global.global_balance' %> + + <%= seconds_to_hm(@organization.try(:global_balance)) %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index a03fcb7db..cda1cfe04 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -281,6 +281,7 @@ en: filter: Filter from: From give_time: Time transfer + global_balance: Global balance home: Home information: Information locales_header: change language From ca70d10ffb37d4c68e63b28a12de90cd6542fd82 Mon Sep 17 00:00:00 2001 From: franpb14 Date: Sat, 15 May 2021 17:00:43 +0200 Subject: [PATCH 2/4] tests: covered the new use case and other tests have been adapted the transfer between users from different organisations has been carefully tested and tests like those of transfer_factory which required a parameter. Issue #638. --- spec/controllers/transfers_controller_spec.rb | 203 ++++++++++++++---- spec/models/transfer_factory_spec.rb | 6 +- spec/views/offers/show.html.erb_spec.rb | 9 +- 3 files changed, 173 insertions(+), 45 deletions(-) diff --git a/spec/controllers/transfers_controller_spec.rb b/spec/controllers/transfers_controller_spec.rb index f7b896cc0..d3c8a7d65 100644 --- a/spec/controllers/transfers_controller_spec.rb +++ b/spec/controllers/transfers_controller_spec.rb @@ -3,6 +3,8 @@ let (:member_admin) { Fabricate(:member, organization: test_organization, manager: true) } let (:member_giver) { Fabricate(:member, organization: test_organization) } let (:member_taker) { Fabricate(:member, organization: test_organization) } + let (:second_organization) { Fabricate(:organization) } + let (:second_member_taker) { Fabricate(:member, organization: second_organization) } describe '#new' do let(:user) { member_giver.user } @@ -51,6 +53,28 @@ end end + context 'when the offer is specified and belongs to another organization' do + let(:other_offer) { Fabricate(:offer, organization: second_organization) } + + it 'finds the transfer offer' do + dest_acc_id = second_member_taker.user.accounts.first.id + get :new, params: params.merge(offer: other_offer.id, + organization_id: other_offer.organization.id, + destination_account_id: dest_acc_id) + + expect(response.body).to include("

#{other_offer}

") + end + + it 'builds a transfer with the offer as post' do + dest_acc_id = second_member_taker.user.accounts.first.id + get :new, params: params.merge(offer: other_offer.id, + organization_id: other_offer.organization.id, + destination_account_id: dest_acc_id) + + expect(response.body).to include("") + end + end + context 'when the offer is not specified' do it 'does not find any offer' do get :new, params: params @@ -142,26 +166,68 @@ } } end - let(:user) { member_admin.user } - - it 'creates a new Transfer' do - expect { post_create }.to change(Transfer, :count).by 1 + subject(:create_between_orgs) do + post 'create', params: { transfer: { + source: member_giver.account.id, + destination: second_member_taker.account.id, + amount: 5 + } } end - it 'creates two Movements' do - expect { post_create }.to change { Movement.count }.by 2 + let(:user) { member_admin.user } + context 'the transfer is within the same organisation' do + it 'creates a new Transfer' do + expect { post_create }.to change(Transfer, :count).by 1 + end + + it 'creates two Movements' do + expect { post_create }.to change { Movement.count }.by 2 + end + + it 'updates the balance of both accounts' do + expect do + post_create + member_giver.reload + end.to change { member_giver.account.balance.to_i }.by -5 + + expect do + post_create + member_taker.reload + end.to change { member_taker.account.balance.to_i }.by 5 + end end - it 'updates the balance of both accounts' do - expect do - post_create - member_giver.reload - end.to change { member_giver.account.balance.to_i }.by -5 - - expect do - post_create - member_taker.reload - end.to change { member_taker.account.balance.to_i }.by 5 + context 'the transfer is between members of different organisations' do + it 'creates three news Transfers' do + expect { create_between_orgs }.to change(Transfer, :count).by 3 + end + + it 'creates six Movements' do + expect { create_between_orgs }.to change { Movement.count }.by 6 + end + + it 'updates the balance of both accounts' do + expect do + create_between_orgs + member_giver.reload + end.to change { member_giver.account.balance.to_i }.by -5 + + expect do + create_between_orgs + second_member_taker.reload + end.to change { second_member_taker.account.balance.to_i }.by 5 + end + + it 'updates the global balance of both organizations' do + create_between_orgs + + expect(test_organization.global_balance).to equal -5 + expect(second_organization.global_balance).to equal 5 + end + + it 'redirects to source user' do + expect(create_between_orgs).to redirect_to(member_giver.user) + end end end @@ -173,30 +239,71 @@ } } end - let(:user) { member_giver.user } - - it 'creates a new Transfer' do - expect { post_create }.to change(Transfer, :count).by 1 - end - - it 'creates two Movements' do - expect { post_create }.to change { Movement.count }.by 2 + subject(:create_between_orgs) do + post 'create', params: { transfer: { + destination: second_member_taker.account.id, + amount: 5 + } } end - it 'updates the balance of both accounts' do - expect do - post_create - member_giver.reload - end.to change { member_giver.account.balance.to_i }.by -5 - - expect do - post_create - member_taker.reload - end.to change { member_taker.account.balance.to_i }.by 5 + let(:user) { member_giver.user } + context 'the transfer is within the same organisation' do + it 'creates a new Transfer' do + expect { post_create }.to change(Transfer, :count).by 1 + end + + it 'creates two Movements' do + expect { post_create }.to change { Movement.count }.by 2 + end + + it 'updates the balance of both accounts' do + expect do + post_create + member_giver.reload + end.to change { member_giver.account.balance.to_i }.by -5 + + expect do + post_create + member_taker.reload + end.to change { member_taker.account.balance.to_i }.by 5 + end + + it 'redirects to destination' do + expect(post_create).to redirect_to(member_taker.user) + end end - it 'redirects to destination' do - expect(post_create).to redirect_to(member_taker.user) + context 'the transfer is to a member of another organizations' do + it 'creates three news Transfers' do + expect { create_between_orgs }.to change(Transfer, :count).by 3 + end + + it 'creates six Movements' do + expect { create_between_orgs }.to change { Movement.count }.by 6 + end + + it 'updates the balance of both accounts' do + expect do + create_between_orgs + member_giver.reload + end.to change { member_giver.account.balance.to_i }.by -5 + + expect do + create_between_orgs + second_member_taker.reload + end.to change { second_member_taker.account.balance.to_i }.by 5 + end + + it 'updates the global balance of both organizations' do + create_between_orgs + + expect(test_organization.global_balance).to equal -5 + expect(second_organization.global_balance).to equal 5 + end + + it 'redirects to source' do + expect(create_between_orgs).to redirect_to(member_giver.user) + end end end end @@ -204,17 +311,31 @@ context 'with invalid params' do let(:user) { member_giver.user } let(:referer) { "/transfers/new?destination_account_id=#{member_taker.account.id}" } + let(:second_referer) { "/transfers/new?destination_account_id=#{member_taker.account.id}&organization_id=#{second_organization.id}" } before do request.env["HTTP_REFERER"] = referer end - it 'does not create any Transfer and redirects to back if the amount is 0' do - expect { - post(:create, params: { transfer: { amount: 0, destination: member_taker.account.id } }) - }.not_to change(Transfer, :count) + context 'the transfer is to a member of the same organization' do + it 'does not create any Transfer and redirects to back if the amount is 0' do + expect { + post(:create, params: { transfer: { amount: 0, destination: member_taker.account.id } }) + }.not_to change(Transfer, :count) + + expect(response).to redirect_to(referer) + end + end + + context 'the transfer is to a member of another organization' do + it 'does not create any Transfer and redirects to back if the amount is 0' do + request.env["HTTP_REFERER"] = second_referer + expect { + post(:create, params: { transfer: { amount: 0, destination: second_member_taker.account.id } }) + }.not_to change(Transfer, :count) - expect(response).to redirect_to(referer) + expect(response).to redirect_to(second_referer) + end end end end diff --git a/spec/models/transfer_factory_spec.rb b/spec/models/transfer_factory_spec.rb index 50310d35a..2e1529e4c 100644 --- a/spec/models/transfer_factory_spec.rb +++ b/spec/models/transfer_factory_spec.rb @@ -4,7 +4,8 @@ organization, current_user, offer_id, - destination_account_id + destination_account_id, + destination_organization_id ) end @@ -12,6 +13,7 @@ let(:current_user) { Fabricate(:user) } let(:organization_offer) { Fabricate(:offer, organization: organization) } let(:destination_account_id) { nil } + let(:destination_organization_id) { organization.id } describe '#offer' do subject { transfer_factory.offer } @@ -32,6 +34,7 @@ let(:offer_id) { organization_offer.id } let(:destination_account_id) { destination_account.id } + let(:destination_organization_id) { organization.id } context 'when the destination account belongs to an organization' do let(:organization) { Fabricate(:organization) } @@ -77,6 +80,7 @@ subject { transfer_factory.transfer_sources } let(:offer_id) { organization_offer.id } + let(:destination_organization_id) { organization.id } let!(:active_member) do Fabricate(:member, organization: organization, active: true) diff --git a/spec/views/offers/show.html.erb_spec.rb b/spec/views/offers/show.html.erb_spec.rb index fce4ba595..8412999e5 100644 --- a/spec/views/offers/show.html.erb_spec.rb +++ b/spec/views/offers/show.html.erb_spec.rb @@ -65,16 +65,18 @@ allow(view).to receive(:current_organization) { another_organization } end - it 'doesn\'t render a link to the transfer page' do + it 'render a link to the transfer page with the id of the destination organisation' do assign :offer, offer + assign :destination_account, destination_account render template: 'offers/show' - expect(rendered).to_not have_link( + expect(rendered).to have_link( t('offers.show.give_time_for'), href: new_transfer_path( id: offer.user.id, offer: offer.id, - destination_account_id: destination_account.id + destination_account_id: destination_account.id, + organization_id: organization.id ) ) end @@ -89,6 +91,7 @@ it 'displays the offer\'s organization' do assign :offer, offer + assign :destination_account, destination_account render template: 'offers/show' expect(rendered).to include( From 1c616709ea412dc52db2d1f6cfb787bef82aa185 Mon Sep 17 00:00:00 2001 From: franpb14 Date: Sun, 16 May 2021 14:12:37 +0200 Subject: [PATCH 3/4] feat: organisations can also transfer time for the offer Sergi says that an organisation can also give time for an offer from another organisation so now it also works if an admin chooses an organisation's account. I have also refactored my code a bit. Issue #638. --- app/controllers/transfers_controller.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/app/controllers/transfers_controller.rb b/app/controllers/transfers_controller.rb index f67ad3be0..68bf80524 100644 --- a/app/controllers/transfers_controller.rb +++ b/app/controllers/transfers_controller.rb @@ -56,14 +56,15 @@ def find_source def create_persisters source_organization = @source.organization.account + source_type = @source.accountable_type destination_organization = @account.organization.account - + @persisters = Array.new if source_organization == destination_organization - @persister = transfer_persister_between(@source, @account) + transfer_persister_between(@source, @account) else - @pers_src_org = transfer_persister_between(@source, source_organization) - @pers_between_orgs = transfer_persister_between(source_organization, destination_organization) - @pers_org_acc = transfer_persister_between(destination_organization, @account) + transfer_persister_between(@source, source_organization) if source_type == "Member" + transfer_persister_between(source_organization, destination_organization) + transfer_persister_between(destination_organization, @account) end end @@ -72,11 +73,11 @@ def transfer_persister_between(source, destination) transfer_params.merge(source: source, destination: destination) ) - ::Persister::TransferPersister.new(@transfer) + @persisters << ::Persister::TransferPersister.new(@transfer) end def persisters_saved? - @persister&.save || @pers_src_org&.save && @pers_between_orgs&.save && @pers_org_acc&.save + @persisters.each { |persister| return false if !persister.save } end def redirect_target @@ -84,7 +85,7 @@ def redirect_target when Organization accountable when Member - accountable.organization == current_organization ? accountable.user : @source.accountable.user + accountable.organization == current_organization ? accountable.user : accountable.organization else raise ArgumentError end From 4f59f9d458a1b6e7137621fbd30073af80a731d6 Mon Sep 17 00:00:00 2001 From: franpb14 Date: Sun, 16 May 2021 14:32:56 +0200 Subject: [PATCH 4/4] tests: added tests for the case where the organisation is the source. Issue #638 --- spec/controllers/transfers_controller_spec.rb | 97 +++++++++++++------ 1 file changed, 70 insertions(+), 27 deletions(-) diff --git a/spec/controllers/transfers_controller_spec.rb b/spec/controllers/transfers_controller_spec.rb index d3c8a7d65..7becc25f8 100644 --- a/spec/controllers/transfers_controller_spec.rb +++ b/spec/controllers/transfers_controller_spec.rb @@ -174,6 +174,14 @@ } } end + subject(:create_between_src_org) do + post 'create', params: { transfer: { + source: test_organization.account.id, + destination: second_member_taker.account.id, + amount: 5 + } } + end + let(:user) { member_admin.user } context 'the transfer is within the same organisation' do it 'creates a new Transfer' do @@ -197,36 +205,71 @@ end end - context 'the transfer is between members of different organisations' do - it 'creates three news Transfers' do - expect { create_between_orgs }.to change(Transfer, :count).by 3 - end - - it 'creates six Movements' do - expect { create_between_orgs }.to change { Movement.count }.by 6 - end - - it 'updates the balance of both accounts' do - expect do - create_between_orgs - member_giver.reload - end.to change { member_giver.account.balance.to_i }.by -5 - - expect do + context 'the transfer is between different organisations' do + context 'the source is a member' do + it 'creates three news Transfers' do + expect { create_between_orgs }.to change(Transfer, :count).by 3 + end + + it 'creates six Movements' do + expect { create_between_orgs }.to change { Movement.count }.by 6 + end + + it 'updates the balance of both accounts' do + expect do + create_between_orgs + member_giver.reload + end.to change { member_giver.account.balance.to_i }.by -5 + + expect do + create_between_orgs + second_member_taker.reload + end.to change { second_member_taker.account.balance.to_i }.by 5 + end + + it 'updates the global balance of both organizations' do create_between_orgs - second_member_taker.reload - end.to change { second_member_taker.account.balance.to_i }.by 5 - end - it 'updates the global balance of both organizations' do - create_between_orgs + expect(test_organization.global_balance).to equal -5 + expect(second_organization.global_balance).to equal 5 + end - expect(test_organization.global_balance).to equal -5 - expect(second_organization.global_balance).to equal 5 + it 'redirects to destination organization' do + expect(create_between_orgs).to redirect_to(second_member_taker.organization) + end end - it 'redirects to source user' do - expect(create_between_orgs).to redirect_to(member_giver.user) + context 'the source is a organization' do + it 'creates two news Transfers' do + expect { create_between_src_org }.to change(Transfer, :count).by 2 + end + + it 'creates four Movements' do + expect { create_between_src_org }.to change { Movement.count }.by 4 + end + + it 'updates the balance of both accounts' do + expect do + create_between_src_org + test_organization.reload + end.to change { test_organization.account.balance.to_i }.by -5 + + expect do + create_between_src_org + second_member_taker.reload + end.to change { second_member_taker.account.balance.to_i }.by 5 + end + + it 'updates the global balance of both organizations' do + create_between_src_org + + expect(test_organization.global_balance).to equal -5 + expect(second_organization.global_balance).to equal 5 + end + + it 'redirects to destination organization' do + expect(create_between_src_org).to redirect_to(second_member_taker.organization) + end end end end @@ -301,8 +344,8 @@ expect(second_organization.global_balance).to equal 5 end - it 'redirects to source' do - expect(create_between_orgs).to redirect_to(member_giver.user) + it 'redirects to destination organization' do + expect(create_between_orgs).to redirect_to(second_member_taker.organization) end end end