Skip to content

add a fincancial balance for each stock taking#1213

Open
mjavurek wants to merge 4 commits into
foodcoops:masterfrom
mjavurek:patch-22
Open

add a fincancial balance for each stock taking#1213
mjavurek wants to merge 4 commits into
foodcoops:masterfrom
mjavurek:patch-22

Conversation

@mjavurek
Copy link
Copy Markdown
Contributor

@mjavurek mjavurek commented Sep 7, 2025

This PR adds a financial balance to each stock taking in the overview:
image

and in the detail view for each taking:
image

So the financial loss (or surplus) of stock takings can be considered in a financial balance.

@mjavurek mjavurek marked this pull request as ready for review September 7, 2025 17:02
@lentschi lentschi self-requested a review February 13, 2026 09:20
Copy link
Copy Markdown
Contributor

@lentschi lentschi left a comment

Choose a reason for hiding this comment

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

@mjavurek Thanks for your contribution!

I've added some code comments.
In addition to those: As a rule of thumb, sums shouldn't be done in the view, but in the controller.

Comment thread app/views/stock_takings/show.html.haml
%td= truncate stock_taking.note
- balance = 0
- for stock_change in stock_taking.stock_changes
- balance += stock_change.quantity * stock_change.stock_article.gross_price
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general, building sums like this may raise performance issues.
If there are a larger number of stock_changes it might be less resource hungry to do this with a db function. Have you considered using sum?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes a db function would be much more effective, I will try to implement it this way. I am not very familiar with this kind of programming and was glad that I could get working it this way on my local foodsoft installation to get the balances for our foodcoop...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mjavurek

I am not very familiar with this kind of programming

Admittedly what I'm suggesting here is quite complicated as we have to take article versioning into account (Something that your stock_change.stock_article automagically did, but with poor performance), so I looked into it myself - Adapting the StockTakings query in the controller's index action like this should work:

@stock_takings = StockEvent
                     .select(%(
                      `#{StockEvent.table_name}`.*,
                      SUM(
                         `#{StockChange.table_name}`.`quantity`
                         * (`#{ArticleVersion.table_name}`.`price` + (`#{ArticleVersion.table_name}`.`deposit`) * ((`#{ArticleVersion.table_name}`.`tax`) + 1))
                      ) AS balance
                     ))
                     .joins(stock_changes: { stock_article: [:article_versions] })
                     .joins(ArticleVersion.latest_outer_join_sql("#{Article.table_name}.#{Article.primary_key}"))
                     .where(later_article_versions: { id: nil })
                     .group('stock_event_id')
                     .order('date DESC')
                     .page(params[:page]).per(@per_page)

Then in the view you can use format_currency(stock_taking.balance) and remove the balance variable you introduced.

Comment thread app/views/stock_takings/_stock_takings.html.haml
Copy link
Copy Markdown
Contributor Author

@mjavurek mjavurek left a comment

Choose a reason for hiding this comment

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

Thanks @lentschi for reviewing my code. Sorry for the missing initialization and my inefficient and unconventional programming style.

@@ -8,11 +8,16 @@
%th= t '.date'
%th= t '.note'
%th
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be a table heading for this new 'balance' column? 🤔

%th= t '.article'
%th= t '.supplier'
%th= t '.unit'
%th
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be a table heading for this new 'balance' column? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants