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

[12.0][FIX] account_budget_oca: fix date format in psql query #26

Merged
merged 3 commits into from Sep 20, 2019

Conversation

cvinh
Copy link
Contributor

@cvinh cvinh commented Jul 30, 2019

Before that fix, we have an error in the query (to_date function does not exist anymore in psql) when we add a budget line and save it

@cvinh cvinh changed the title [FIX] fix date format in psql query [12.0][FIX] account_budget_oca: fix date format in psql query Jul 31, 2019
Copy link
Member

@marcelsavegnago marcelsavegnago left a comment

Choose a reason for hiding this comment

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

Revised and tested

@pedrobaeza pedrobaeza added this to the 12.0 milestone Aug 9, 2019
AND (date between to_date(%s,'yyyy-mm-dd')
AND to_date(%s,'yyyy-mm-dd'))
AND (date between %s
AND %s)
Copy link
Member

Choose a reason for hiding this comment

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

Looking at these lines it seems that wizard_date_from and wizard_date_to are expected to be strings. In this case the to_date() is still necessary.
I think you should keep the to_date() and convert the line.date_from and line.date_to to string.

Copy link
Member

Choose a reason for hiding this comment

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

Another solution would be to remove all the occurrences of wizard_date_from and wizard_date_to. It seems they are not used, probably they are a leftover from old code. This way you could keep your solution that removes the to_date().

Copy link
Contributor Author

@cvinh cvinh Aug 28, 2019

Choose a reason for hiding this comment

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

Yes it looks like wizard_date_from and wizard_date_to are unused... I didn't search where that codes come from...
By the way the aim of this PR is that to_date function does not exist anymore in Postgresql since... v9.6 ? v10.0 ?
(in our case v10)

Copy link
Member

Choose a reason for hiding this comment

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

OK then! Let's go with the fix you proposed. But please remove all the occurrences of wizard_date_from and wizard_date_to because they are not used in the code and in any case they would not work.

Choose a reason for hiding this comment

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

@cvinh @astirpe hello, I was looking at this module and received the error this issue was created for, is this issue stale or is it still being worked?

@cvinh
Copy link
Contributor Author

cvinh commented Sep 10, 2019

Hello, on our side we use the PR in production @AStripe fell free to override it if you think code should be cleaned up... sorry did not have time to go further on that one

@astirpe
Copy link
Member

astirpe commented Sep 10, 2019

@cvinh Is not just a matter of cleaning up. After your fix, the part of code I was mentioning (wizard_date_from and wizard_date_to) will be failing if someone use it for some reason.

@cvinh
Copy link
Contributor Author

cvinh commented Sep 10, 2019

I see... let me do that changes... it's not that big

@patrickrwilson
Copy link

Thanks @cvinh, I look forward to test and review once you submit the update.

Copy link

@patrickrwilson patrickrwilson left a comment

Choose a reason for hiding this comment

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

Good work, tested and works great.

@astirpe
Copy link
Member

astirpe commented Sep 20, 2019

Can this one be merged?

@pedrobaeza
Copy link
Member

Merging blindly according reviews

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-26-by-pedrobaeza-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 20, 2019
Signed-off-by pedrobaeza
@OCA-git-bot OCA-git-bot merged commit c1e1412 into OCA:12.0 Sep 20, 2019
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 2f03602. Thanks a lot for contributing to OCA. ❤️

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

Successfully merging this pull request may close these issues.

None yet

6 participants