multi transfers

[Morantron]

Closes #469

Wat

Multi transfers feature according to:

https://community.coopdevs.org/t/transferencias-de-horas-a-multiples-usuarios/807/2

The implementation consists of two main pieces:

• An old-school server-side rendered form, in a multi-step wizard fashion.
• A set of services objects that manages the creation of the actual transfers in DB.

The form

The form backend

A form divided in multiple steps. All steps are rendered, but only the current one is displayed through CSS.

The wizard starts in this URL

We use GET here so we can link to the initial step with an ordinary link

GET /multi/step/1

The action of the <form /> element points to the next step, and steps are propagated through form data using POST method/

We use POST here to propagate params

POST /multi/step/2
POST /multi/step/3

etc...

The form detects if it's in the last step, to point to the controller action that actually creates the transfers.

The form frontend

I had to add some JS ( SORRY ENRICO ).

I'm reusing jQuery select2 plugin, to mimic pre-existing behaviour when selecting the source/target of transfers. I had to address some jQuery weirdness ( old school TM ), isolated and documented into a separate file multi_select2.js.

Operation services

There are three type of operations ( although only 2 are used ).

• OneToOne
• ManyToOne
• OneToMany

All these are implemented by subclassing Operations::Transfers::Base. The only differing behaviour is where/from the transfers must happen ( check base class for moar docs ). Base class contains common behavior of creating the Transfer objects within a transaction.

There is also a factory function, which receives an array of ids called from, to. Based on the length of the arrays it's able to determine what type of operation it should instantiate.

[sseerrggii]

#469

[markets]

@Morantron looks really nice in general 👍, to be honest, I don't know well this part (transfers), so I just added some "high level" comments (most important is the one related to front-end validations ✂️).

Also, I want to add a couple of comments:

• Travis is failing 🔴
• "There are three type of operations ( although only 2 are used )." => can we just implement (and maintain) the 2 that are really used? It will also reduce the diff.

[markets]

@Morantron why this check + front-end validations were removed? We introduced it recently, in #421

[markets]

@Morantron @sseerrggii I think we should leave this front-end validation working for both scenatios: regular transfer and multi-transfers. We have back-end and DB constraints, but I'd like to keep this friendly i18n front-end validation 👌

timeoverflow/app/views/transfers/new.html.erb

Line 40 in 5093f0c

<label class="error invisible"><%= t ".error_amount" %></label>

If you try a multi-transfer with amount 0, probably you will see some ROLLBACK ⚠️ in the log.

Regular transfer example 👇

[Morantron]

I've rearranged the code to preserve this behavior. I only need the bits that fill the hidden input with the total amount of seconds ( hours + minutes ). Validation logic breaks the multi transfer wizard, since we always render the field but it might not be filled yet.

[markets]

@Morantron I think it's important to keep this front-end validation, I just tried locally this branch and I get the following weird behavior:

1. I fill with amount 0 (0 hours, 0 mins)
2. Transfers created successfully as a flash message
3. I see a db ROLLBACK in the log and the transfer isn't created at all 😲

That's really confusing, IMHO we should improve it before shipping it :)

Maybe we can try something like: perform js validation only in specific step.

[Morantron]

I'll add some controller/feature tests, and everything will be verdura/green 🙌

[markets]

@Morantron only a couple of items from my side 🙏 😄

• Amount validation (discussion here 👉 #460 (comment))
• I think we should add more validations to these multi steps form. Example: now I can leave from or to empty via the UI, and at the end of the process I get 500 error:

NoMethodError - undefined method `length' for nil:NilClass:
  app/services/operations/transfers.rb:14:in `transfer_klass'
  app/services/operations/transfers.rb:6:in `create'
  app/controllers/multi_transfers_controller.rb:19:in `create'

[Morantron]

@markets true dat, I think I've fixed the validation issues, try again plz 🙏

[markets]

@Morantron just 1 missing weird behavior (and crash 💥) now:

• select "Many to one" type
• from: select only 1
• to: select 1

It crashes with:

ArgumentError - Unknown type of transfer:
  app/services/operations/transfers.rb:19:in `transfer_klass'
  app/services/operations/transfers.rb:6:in `create'
  app/controllers/multi_transfers_controller.rb:19:in `create'

[Morantron]

Fixed the broken case, which is covered by the OneToOne transfer operation ( i brought it back ).

It's all green now

[markets]

Cool @Morantron 👌 ✅

2.4% extra coverage 🔝 😸

[enricostano]

👏

[enricostano]

@sseerrggii we're ready to give it a final manual test. Please deploy it to staging and test it whenever you can. Also take in account Marc's latest comments like #460 (comment)

[sseerrggii]

Testing I noticed that as no-admin user I can access through URL https://staging.timeoverflow.org/multi/step/1 and create transfers 😱

Can we ensure on backend that normal users only can pay from their accounts? 🙏

Also can we use key title something like multi_transfers? because we will have some languages pending to translate and english is better than nothing

everything else seems to work perfectly 💪

[Morantron]

added multi transfer policy to prevent regular users from accessing the controller and changed that copy 💃

[sseerrggii]

@Morantron you forget to change <h1> title in view 89efb95

[sseerrggii]

Now everything is ok 🎉

[enricostano]

@sseerrggii @Morantron can we remove the needs testing tag? Are we ready to merge?

ARE YOU SURE?

[sseerrggii]

Yes @enricostano ! Tested ✔️