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

[16.0][IMP] account_reconcile_oca: verify data is defined #584

Closed
wants to merge 1 commit into from

Conversation

etobella
Copy link
Member

@etobella etobella commented Sep 4, 2023

Should fix #579 (comment)

@pedrobaeza pedrobaeza added this to the 16.0 milestone Sep 4, 2023
@pedrobaeza pedrobaeza changed the title [IMP] account_reconcile_oca: verify data is defined [16.0][IMP] account_reconcile_oca: verify data is defined Sep 4, 2023
@pedrobaeza
Copy link
Member

Can't you prepare data so line_currency_id is always filled?

@etobella
Copy link
Member Author

etobella commented Sep 4, 2023

The problem might come from old data. If you had this module installed and you reconciled a line previous to the last version, data will not be there. The only option I find is to do this 🤔 migrating the lines might be hard and expensive for just this... WDYT?

@pedrobaeza
Copy link
Member

But in any place should be filling line dictionary, so I'm talking to fill that value always in that place instead of having to be defensive each time you access data.

@etobella
Copy link
Member Author

etobella commented Sep 5, 2023

I understand it, and we are doing that. The problem comes from old data that might have been generated before the inclussion of such data. If you reset and old line, the json might not contain the data 😭

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

OK if there's no other solution. In general, storing intermediate data instead of preparing them on the fly is not a good design choice, but not knowing the details, I can't totally judge.

@etobella
Copy link
Member Author

etobella commented Sep 5, 2023

Mmmh, I get your point @pedrobaeza I will create a second PR fixing it in another way, let's see what's better 😄

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