Move give time action to transfers controller #264

sauloperez · 2017-09-19T11:05:22Z

This removes the duplicated action #give_time (along with its views) in users and organizations controllers and moves it to the TransfersController#new.

Missing

Couple screenshots
Test and docs for TransferFactory
Update current :give_time links for organization

This duplicates the UsersController#give_time to TransferController#new as it should be by Rails conventions. We aim to stick to REST conventions of the Transfer resource instead of having custom actions on users and organizations controllers.

This duplicates the OrganizationsController#give_time to TransferController#new as it should be by Rails conventions. We aim to stick to REST conventions of the Transfer resource instead of having custom actions on users and organizations controllers.

This is slowly turning "turning difference into sameness" following Sandi Metz style.

By specifying the account id we can abstract away from User or Organization objects and trust just the account. As a result, this removes ifs and therefore complexity.

Then each particular view knows what type the accountable has and uses the appropriate methods like user_path or organization_path.

This makes the controller specs integration-like enough to ensure things keep working when we later refactor the views and the controller ivars.

Now transfers/new.html.erb and transfers/new_organization.html.erb are identical. All their differences were moved to partials as if we extracted abstract methods in a class and moved the conditionals within them.

Now there's no point on using one for user/member and another one for organization.

enricostano · 2017-09-19T13:57:19Z

app/models/transfer_sources_options.rb

@@ -0,0 +1,26 @@
+class TransferSourcesOptions


Doc please 😬

enricostano

Doc++ but 👏

enricostano · 2018-01-04T15:11:26Z

app/models/account_option_tag.rb

+    accountable.display_id(destination_accountable)
+  end
+end
+


enricostano · 2018-01-04T15:11:36Z

app/models/account_option_tag.rb

@@ -0,0 +1,19 @@
+class AccountOptionTag


sauloperez · 2018-01-04T15:39:46Z

I'll add any missing bits of doc but essentially this is what this first refactor is all about. As it stands out now, there are some odd pieces that need to be worked on in an upcoming PR.

sauloperez · 2018-01-04T15:41:58Z

app/models/transfer_factory.rb

@@ -0,0 +1,79 @@
+class TransferFactory


This is the first try at encapsulating all this transfer building logic. It's more of a patch to let us move code around until we figure out how the data model really works.

sauloperez · 2018-01-04T15:42:37Z

app/views/organizations/give_time.html.erb

-    <% if admin? %>
-      <div class="form-group">
-        <%= f.label :source %>
-        <%# TODO: refactor. it's very complex for a view. %>


Most of this complexity it's been moved to TransferSourcesOptions

sauloperez · 2018-01-04T15:44:44Z

app/models/organization.rb

@@ -53,4 +57,12 @@ def ensure_url
      errors.add(:web, :url_format_invalid)
    end
  end
+
+  def display_id(destination_accountable)
+    if destination_accountable.is_a?(Organization)


There are quite a lot of this type checking on Organization. Very smelly 👃 so I guess this should be our next focus.

sauloperez · 2018-01-04T15:50:15Z

app/controllers/transfers_controller.rb

+      current_organization,
+      current_user,
+      params[:offer],
+      params[:destination_account_id]


The term destination_account would be better off as payee. Seems more appropriate to this domain.

sauloperez · 2018-01-05T08:23:18Z

@sseerrggii could you please test this? it affects both, transfers to a member and to an organization.

enricostano · 2018-01-18T16:03:10Z

app/controllers/transfers_controller.rb

+        accountable: accountable,
+        transfer: transfer,
+        offer: offer,
+        sources: transfer_sources


What about directly:

{ accountable: transfer_factory.accountable, transfer: transfer_factory.build_transfer, offer: transfer_factory.offer, sources: transfer_factory.transfer_sources }

enricostano · 2018-01-19T10:36:31Z

app/helpers/transfers_helper.rb

+    else
+      user_path(accountable.user)
+    end
+  end


Is there any reason why this can't be an AccountableMixin extending Organization and User models? Could we share an interface to expose the path?

I'm not sure. I'd just do whatever it takes to follow Rails convention so that we don't have to define any path helper. But because we made this standout, we can work on it in an upcoming iteration, not here.

sauloperez · 2018-01-22T14:42:25Z

If you agree @enricostano I'll prepare this branch for @sseerrggii to test it. My work here is done.

enricostano · 2018-01-22T17:15:23Z

spec/controllers/organizations_controller_spec.rb

@@ -0,0 +1,14 @@
+require "spec_helper"


Single quote maybe

enricostano

👏

sseerrggii · 2018-01-26T14:38:37Z

Tested -> Fine :)

sauloperez added 16 commits August 16, 2017 18:53

DRY #new by abstracting ivars into their methods

808ed91

This is slowly turning "turning difference into sameness" following Sandi Metz style.

Autofix cops and change style of spec

69982ab

Simplify #create action

0aa4073

Remove dead #give_time actions

0fa4668

Abstract transfers controller by passing account

fa3adf4

By specifying the account id we can abstract away from User or Organization objects and trust just the account. As a result, this removes ifs and therefore complexity.

Use @accountable instead of @user or @organization

24e5d6a

Then each particular view knows what type the accountable has and uses the appropriate methods like user_path or organization_path.

Refactor assigns' specs to check HTML instead

96f36b1

This makes the controller specs integration-like enough to ensure things keep working when we later refactor the views and the controller ivars.

Replace ivars with locals in views

b50d662

Extract transfer sources select

bf6223b

Remove differences between new, new_organization

a624dbc

Now transfers/new.html.erb and transfers/new_organization.html.erb are identical. All their differences were moved to partials as if we extracted abstract methods in a class and moved the conditionals within them.

Use transfers :new template only

fc902af

Now there's no point on using one for user/member and another one for organization.

Extract option building into class + polymorphism

14eaad8

Extract TransferFactory and decouple view

be04d97

Replace conditional with #find_by_id

e97d467

enricostano reviewed Sep 19, 2017

View reviewed changes

Add specs and docs to TransferSourceOptions

5f4d70e

sauloperez force-pushed the move-give-time-action-to-transfers-controller branch from dc30890 to 5f4d70e Compare January 4, 2018 14:49

enricostano suggested changes Jan 4, 2018

View reviewed changes

sauloperez added 3 commits January 4, 2018 16:28

Inline AccountOptionTag to avoid wrong abstraction

7b51848

Remove obsolete :give_time action views

95230dc

Fix doc

b319ed9

sauloperez commented Jan 4, 2018

View reviewed changes

Add test and doc to #display_id

b13b38e

sauloperez mentioned this pull request Jan 11, 2018

mail notification to origin and destination when transfer is done #247

Closed

sseerrggii added the needs testing label Jan 11, 2018

sauloperez assigned sseerrggii Jan 15, 2018

Add specs to TransferFactory

b71dfb7

enricostano reviewed Jan 18, 2018

View reviewed changes

Replace give_time link from users#show page

47812a9

enricostano reviewed Jan 19, 2018

View reviewed changes

sauloperez added 2 commits January 22, 2018 15:29

Replace give_time link from organizations#show

e40a0ff

Remove intermediate vars

545ea44

enricostano suggested changes Jan 22, 2018

View reviewed changes

spec/controllers/organizations_controller_spec.rb Outdated

@@ -0,0 +1,14 @@

require "spec_helper"

Copy link

Contributor

enricostano Jan 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single quote maybe

Fix some rubocop violations

70efd91

sauloperez force-pushed the move-give-time-action-to-transfers-controller branch from 543c222 to 70efd91 Compare January 22, 2018 17:21

enricostano approved these changes Jan 22, 2018

View reviewed changes

sauloperez assigned sauloperez and unassigned sauloperez Jan 22, 2018

sauloperez mentioned this pull request Jan 25, 2018

Rename destination_account to payee #296

Closed

sauloperez removed the needs testing label Jan 26, 2018

sauloperez merged commit ca5e813 into develop Jan 26, 2018

sauloperez deleted the move-give-time-action-to-transfers-controller branch January 26, 2018 15:51

sauloperez mentioned this pull request Jan 30, 2018

Release January 2018 / 1 #303

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move give time action to transfers controller #264

Move give time action to transfers controller #264

sauloperez commented Sep 19, 2017 •

edited

Loading

enricostano Sep 19, 2017

sauloperez Sep 19, 2017

sauloperez Jan 4, 2018

enricostano left a comment

enricostano Jan 4, 2018

enricostano Jan 4, 2018

sauloperez commented Jan 4, 2018

sauloperez Jan 4, 2018 •

edited

Loading

sauloperez Jan 4, 2018

sauloperez Jan 4, 2018

sauloperez Jan 4, 2018

sauloperez commented Jan 5, 2018 •

edited

Loading

enricostano Jan 18, 2018

sauloperez Jan 19, 2018

enricostano Jan 19, 2018

sauloperez Jan 19, 2018

sauloperez commented Jan 22, 2018 •

edited

Loading

enricostano Jan 22, 2018

enricostano left a comment

sseerrggii commented Jan 26, 2018

Move give time action to transfers controller #264

Move give time action to transfers controller #264

Conversation

sauloperez commented Sep 19, 2017 • edited Loading

Missing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enricostano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez commented Jan 4, 2018

sauloperez Jan 4, 2018 • 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

sauloperez commented Jan 5, 2018 • 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

sauloperez commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

enricostano left a comment

Choose a reason for hiding this comment

sseerrggii commented Jan 26, 2018

sauloperez commented Sep 19, 2017 •

edited

Loading

sauloperez Jan 4, 2018 •

edited

Loading

sauloperez commented Jan 5, 2018 •

edited

Loading

sauloperez commented Jan 22, 2018 •

edited

Loading