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
Add reference_calculator #565
Conversation
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.
Interesting!
I fail to fully understand what is happening here. It looks like you expect each member to validate their own transactions. The javascript does something with that, apparently.
Could you eloborate a bit more what the intended high-level flow is, please?
}); | ||
|
||
= form_tag finance_create_transaction_collection_path do | ||
= hidden_field_tag 'prefix', "FS#{@current_user.ordergroup.id}.#{@current_user.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.
this seems pretty hardcoded, would be good to make that more flexible, preferably on some model
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 need to be able to parse it later too, i see no better way than hardcoding it
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.
What about using a helper class for encoding and decoding those strings? I understand the JS part is then a bit of an issue, so either we could compute it server-side, or include a method in the file with the js-computation part (only the computation part, not the display part).
Also, I would really suggest to add a check digit somewhere (perhaps optional, but for generated ones this helps detecting cut-n-paste errors or typos). But can be a feature to add later.
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 really don't like to calculate it on the server side, but we can but the prefix in the global config. please check out #568 for the parsing part, so we can sync the ideas
add a check digit somewhere
it's important that this format can be generated by humans too. they only need to know the number of the ordergroup (which stays the same all the time). As long as the check digit is optional it does not provide a real value and we have a kind of check already, since the sum of the amounts in the reference must match the amount of the bank transaction
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.
Ok, fine with the no-check-digit route.
I still think putting the reference computation logic in one place would help in the future, and shouldn't be so involved, right? I don't know, but somehow I'd like to keep this a clearly defined piece, so it can be customized by foodcoops / plugin authors.
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.
doing the stuff in a CLEAN way on the server is far from trivial and i don't see the benefits outweigh the effort. If someone will ever implement a plugin, she can update the existing code when needed. i don't want to over engineer it.
maybe you can give #568 a look before we continue 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 agree about doing everything server-side, that's too involved.
But what about adding a module in lib/
that
- converts fields (hash?) to a reference (string)
- converts a reference (string) to fields (hash?)
- returns the javascript needed to convert fields (js object?) to a reference (string)
That would be easy and give flexibility.
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.
The js could be something like (just a quick try):
function(prefix, values) {
var amount = 0;
var reference = prefix;
values.each(function(o) {
var name = o.name;
var value = Math.round(parseInt(o.value, 10));
if (value > 0) {
amount += value;
reference += name + value;
}
});
return { amount: amount, reference: reference };
}
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'll give it a try, but i like to get at least an initial review on #568 first, since there might be some overlapping points, which i'd like to implement together
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.
One other thing: I'd consider moving the function to an asset, so it can be more easily overridden. Maybe set the function on window.foodsoftParseBankTransactionReference
? Or make the function name a controller instance variable, so it can be overridden? Deface has issues overriding Javascript in places like this.
Example two users 1 in ordergroup 10 and user 2 in odergroup 20. we have two financial transaction types A an B if we find a bank transaction witch matches FS10.1A100 with an amount of 100 EUR, we can create a transaction A with 100EUR at ordergroup 10. For "FS20.2A30B70" we can automatically create two financial transactions for ordergroup 20: A with 30EUR and B with 70 EUR. This reference calculator helps the members to create the correct string, if they do not get how the system works (or don't know their prefix). |
e499905
to
0c9378e
Compare
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.
Keep it up :)
@@ -9,6 +9,8 @@ class FinancialTransactionType < ActiveRecord::Base | |||
|
|||
before_destroy :restrict_deleting_last_financial_transaction_type | |||
|
|||
scope :with_name_short, -> { where.not(name_short: '') } |
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 guess where.not(name_short: [nil, ''])
would be safer here?
@@ -9,6 +9,8 @@ | |||
%ul.dropdown-menu | |||
%li= link_to t('.profile'), my_profile_path | |||
%li= link_to t('.ordergroup'), my_ordergroup_path | |||
- if BankAccount.any? | |||
%li= link_to t('.reference_calculator'), home_reference_calculator_path |
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 guess people would use bank accounts without the reference stuff you're using?
What about a configuration option for using the references to direct where the money is for? Or at the very least, if there is a name_short
somewhere ... but that would need some caching perhaps, since this is called on every page view.
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.
This can also be done later, when someone requests it. Fine.
}); | ||
|
||
= form_tag finance_create_transaction_collection_path do | ||
= hidden_field_tag 'prefix', "FS#{@current_user.ordergroup.id}.#{@current_user.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.
One other thing: I'd consider moving the function to an asset, so it can be more easily overridden. Maybe set the function on window.foodsoftParseBankTransactionReference
? Or make the function name a controller instance variable, so it can be overridden? Deface has issues overriding Javascript in places like this.
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.
After a long break, I've found some time to review this again. Looking good, feel free to merge!
No description provided.