Skip to content
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

Transfers with different currencies are incorrectly updated during database update #724

Closed
skibbipl opened this issue Jul 27, 2017 · 16 comments
Assignees
Labels
bug Verified and replicated bugs and issues. fixed Bugs that are fixed (in a coming release).

Comments

@skibbipl
Copy link

Hi,
I've upgraded from 4.6.1 to 4.6.3.1 today. During the upgrade procedure following info was displayed:
Transfer #284 has been updated to use the correct currencies. Please verify this in Firefly III.
I've checked the mentioned transfer and it was updated incorrectly! Before upgrade transfer looked like this (an example numbers):
Total amount | $500.00 (€400.00)
After the upgrade the same transfer was like this:
Total amount | $500.00 (€500.00)
It seems that during upgrade firefly decided to use 1:1 currency conversion rate. It seems like a bug.

Regards

@JC5
Copy link
Member

JC5 commented Jul 27, 2017

I'm looking into it!

@JC5
Copy link
Member

JC5 commented Jul 27, 2017

I'm sorry, I can't replicate this. I've created a EUR and a USD asset account in a fresh 4.6.1 installation, and I transfer some money between them. I then insert the database into a 4.6.3.1 installation and tell it to upgrade.

The only change is that the journal_meta values for foreign_amount and foreign_currency_id are gone.

Otherwise, everything else seems to be the same.

@skibbipl
Copy link
Author

OK, I don't have many transfers between different currencies so let's hope future updates will not change the amounts.

@skibbipl skibbipl reopened this Aug 30, 2017
@skibbipl
Copy link
Author

skibbipl commented Aug 30, 2017

Well, the issue popped again when I wanted to upgrade from 4.6.3.1 to 4.6.4. Hopefully I did this on the copy of the production database. Will be glad to help you debug the issue further. Any SQL-s that I can make on prod and backup databases for comparison?
Unfortunately this time I have a lot more transactions with different currencies so I will prefer not to update them every time I upgrade firefly.

@skibbipl skibbipl changed the title Updating currencies during upgrade Transfers with different currencies are incorrectly updated during database update Aug 30, 2017
@JC5
Copy link
Member

JC5 commented Aug 30, 2017

Damn :(. I really hoped this was fixed.

What I would like you to run is the following:

select * from accounts where id in (1,2,3); for all involved asset accounts.
select * from account_meta where id in (1,2,3); for all involved asset accounts.

select * from transaction_journals where id in (1,2,3) for the transfers that are not converted correctly.
select * from journal_meta where transaction_journal_id in (1,2,3) for the transfers that are not converted correctly.

select * from transactions where transaction_journal_id in (1,2,3) for the transfers that are not converted correctly.

That should give me enough information to fix the upgrade-routine. Please censor the data as you like and send me the results by mail

@skibbipl
Copy link
Author

Mails sent and just a reminder - I use PostgreSQL.

@JC5
Copy link
Member

JC5 commented Sep 3, 2017

I don't understand why the firefly:upgrade routine doesn't give your accounts a preferred currency_id.

@skibbipl
Copy link
Author

skibbipl commented Sep 3, 2017

The upgrade sets preferred currency id for some accounts according to the messages.

@JC5
Copy link
Member

JC5 commented Sep 6, 2017

I've created a fresh 4.6.1 installation just now and I'm doing some extended debugging. I'm going to upgrade from 4.6.1 to 4.6.3.1 to 4.6.4 (dev version).

Of course on Postgres, and with as many foreign transactions as I can think of ^^

@JC5
Copy link
Member

JC5 commented Sep 6, 2017

I'm currently back at 4.6.0 but it all seems to work fine. Could you edit your asset accounts, verify their currency preference and save them? The table account_meta must have a entry called currency_id for each asset account.

@skibbipl
Copy link
Author

skibbipl commented Sep 6, 2017

Hi, I double checked all asset accounts and each have correct currency set and also corresponding entry in account_meta table. Despite that the upgrade procedure keeps updating the transfers with 1:1 ratio. Maybe the upgrade procedure is related somehow to server locale or database locale? I'm using pl_PL.UTF-8 both for LANG and database collation. I've checked the source code and this line is suspicious to me:

$journal->setMeta('foreign_amount', $destinationTransaction->amount);

Why are you setting foregin_amount of the destination transaction with the amount of the destination transaction? Shouldn't this line looks like this:

$journal->setMeta('foreign_amount', $destinationTransaction->foreign_amount);

@JC5
Copy link
Member

JC5 commented Sep 7, 2017

OK cool, then it's the 1:1 ratio I can focus on. I know that the current routine also uses a 1:1 conversion which isn't too bad per se. It should however:

  • also verify what the journal_meta tables mention about foreign amounts and use that.
  • also verify if the foreign_amount is not already filled in.

@JC5 JC5 self-assigned this Sep 8, 2017
@JC5 JC5 added the bug Verified and replicated bugs and issues. label Sep 8, 2017
JC5 added a commit that referenced this issue Sep 8, 2017
@JC5
Copy link
Member

JC5 commented Sep 8, 2017

I think I fixed this, I tried many variations and FF comes out on top all the time. However, sometimes there is no choice but to do 1:1 conversion.

I will release the latest FF3 this weekend and I hope this will finally be solved. Make sure you log on the DEBUG level and keep an eye on the log files.

@JC5 JC5 added the fixed Bugs that are fixed (in a coming release). label Sep 8, 2017
@JC5
Copy link
Member

JC5 commented Sep 9, 2017

New release is live, closed.

@JC5 JC5 closed this as completed Sep 9, 2017
@skibbipl
Copy link
Author

skibbipl commented Sep 9, 2017

No errors during upgrade. All transactions have correct currency ratio. Thank you for debugging this!

@JC5
Copy link
Member

JC5 commented Sep 9, 2017

Glad to hear it. No problem! 👍

@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Verified and replicated bugs and issues. fixed Bugs that are fixed (in a coming release).
Projects
None yet
Development

No branches or pull requests

2 participants