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.

Sign up for GitHub

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

Jump to bottom

[16.0][MIG] pos_session_pay_invoice: Migration to version 16.0 #1226

Merged
merged 22 commits into from
Sep 25, 2024
Merged

[16.0][MIG] pos_session_pay_invoice: Migration to version 16.0 #1226

merged 22 commits into from
Sep 25, 2024

Conversation

carlos-lopez-tecnativa
Copy link
Contributor

@Tecnativa TT49807

legalsylvain
legalsylvain reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi @carlos-lopez-tecnativa.

Thanks for porting this module tecnically, I don't understand why

  • some button open a wizard that inherit from cash.pay.invoice,
  • and in other button, it open a new model pos.box.cash.invoice.in / pos.box.cash.invoice.out.

could you elaborate ?

pos_session_pay_invoice/readme/ROADMAP.rst Outdated Show resolved Hide resolved
@carlos-lopez-tecnativa
Copy link
Contributor Author

Hi @carlos-lopez-tecnativa.

Thanks for porting this module tecnically, I don't understand why

  • some button open a wizard that inherit from cash.pay.invoice,
  • and in other button, it open a new model pos.box.cash.invoice.in / pos.box.cash.invoice.out.

could you elaborate ?

Hi @legalsylvain,

In this module, I have only migrated and made minimal changes for now. However, if you consider it necessary, I can refactor it similarly to this PR to inherit from the new wizard cash.pay.invoice for customer/vendor invoices instead of using two separate wizards.

Before the referenced PR, these wizards (cash.invoice.in, cash.invoice.out) inherited from cash.box.out, but as it no longer exists, I created a single, more generic wizard called cash.pay.invoice. In this module, the wizard that depends on cash.invoice.in is updated to inherit from cash.pay.invoice. However, the wizards pos.box.cash.invoice.out and pos.box.cash.invoice.in do not inherit from any, so I didn't touch them.

Let me know if the refactor is welcome.

@legalsylvain
Copy link
Contributor

Hi @carlos-lopez-tecnativa.
Sorry. I didn't understood. I tested a little locally.
Functionaly, why there are two different ways to do the same thing ?

@carlos-lopez-tecnativa
Copy link
Contributor Author

Hi @carlos-lopez-tecnativa. Sorry. I didn't understood. I tested a little locally. Functionaly, why there are two different ways to do the same thing ?

@legalsylvain That was the behavior in the previous version; there were several wizards to do each thing. Now, based on my previous comment and the referenced PR(OCA/account-payment#743), we could simplify this. Let me know if that works for you. @pedrobaeza what do you think?

@pedrobaeza
Copy link
Member

Yes, please simplify it.

@legalsylvain
Copy link
Contributor

Agree with Pedro. Let me know when you want review.
Thanks for the work !

@carlos-lopez-tecnativa
Copy link
Contributor Author

carlos-lopez-tecnativa commented Aug 13, 2024 •
edited
Loading

@pedrobaeza @legalsylvain I have updated the code to inherit functionality from the cash.pay.invoice wizard and removed the older wizards. Please review the changes and let me know if they work fine for you.

@pedrobaeza
Copy link
Member

/ocabot migration pos_session_pay_invoice

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Aug 14, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Aug 14, 2024
Open
37 tasks
pedrobaeza
pedrobaeza reviewed Aug 14, 2024
View reviewed changes
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.

I can't find the buttons specified in the README in the POS session.

pos_session_pay_invoice/README.rst Outdated Show resolved Hide resolved
@pedrobaeza
Copy link
Member

I don't understand why limiting the invoice payment to cash methods. Someone can come to your physical store to pay an invoice, and choose to pay with credit card. I think that limitation should be gone.

@pedrobaeza
Copy link
Member

And anyway, even adding a cash payment method, I don't find the buttons referred in the README

@carlos-lopez-tecnativa
Copy link
Contributor Author

And anyway, even adding a cash payment method, I don't find the buttons referred in the README

@pedrobaeza the buttons are here. Could you attach an image, please?
image

image

@pedrobaeza
Copy link
Member

I was looking in the PoS session "frontend". In any moment the README mentions that you should go to the session backend form.

@carlos-lopez-tecnativa
Copy link
Contributor Author

I don't understand why limiting the invoice payment to cash methods. Someone can come to your physical store to pay an invoice, and choose to pay with credit card. I think that limitation should be gone.

This behavior is from the previous version. Do you want us to change it to allow the user to select any payment method available on POS? (But only if the payment method has a journal set.) Please confirm so I can adapt the code.

@pedrobaeza
Copy link
Member

OK, I see the functionality very limited having to go to backend, so let's not invest more time in this. Please clarify the usage README though.

@carlos-lopez-tecnativa
Copy link
Contributor Author

OK, I see the functionality very limited having to go to backend, so let's not invest more time in this. Please clarify the usage README though.

Odoo, in the new version, has moved most functionalities to the frontend. This implies more effort to change it to the frontend. The task for now is to migrate the current module.
I have updated the readme. Please let me know if it is clearer now. Thanks.

pedrobaeza
pedrobaeza approved these changes Aug 14, 2024
View reviewed changes
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.

Code and functional review

@carlos-lopez-tecnativa
Copy link
Contributor Author

@legalsylvain, could you please review again?

@legalsylvain
Copy link
Contributor

Hi @carlos-lopez-tecnativa. Thanks for the ping ! I just come back from hollidays today. I take a look this week.

thanks !

@carlos-lopez-tecnativa
Copy link
Contributor Author

Hi @carlos-lopez-tecnativa. Thanks for the ping ! I just come back from hollidays today. I take a look this week.

thanks !

@legalsylvain I hope you had a great holiday! Would you kindly take another look when you have a chance? I'd appreciate any updates or feedback you may have.

ivantodorovich
ivantodorovich approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

code review LG

zamberjo and others added 7 commits September 19, 2024 07:47
@zamberjo @carlos-lopez-tecnativa
[FIX] search_paid_order_ids must exclude orders createds with this …
1ac3586 
…module
@carlos-lopez-tecnativa
[UPD] Update pos_session_pay_invoice.pot
52e7b73
@OCA-git-bot @carlos-lopez-tecnativa
pos_session_pay_invoice 14.0.1.1.0
c215740
@anmarmo1 @carlos-lopez-tecnativa
Translated using Weblate (Spanish)
c36d996 
Currently translated at 86.6% (26 of 30 strings)

Translation: pos-14.0/pos-14.0-pos_session_pay_invoice
Translate-URL: https://translation.odoo-community.org/projects/pos-14-0/pos-14-0-pos_session_pay_invoice/es/
@Ivorra78 @carlos-lopez-tecnativa
Translated using Weblate (Spanish)
c6ed5a1 
Currently translated at 100.0% (30 of 30 strings)

Translation: pos-14.0/pos-14.0-pos_session_pay_invoice
Translate-URL: https://translation.odoo-community.org/projects/pos-14-0/pos-14-0-pos_session_pay_invoice/es/
@mymage @carlos-lopez-tecnativa
Translated using Weblate (Italian)
2393c61 
Currently translated at 100.0% (30 of 30 strings)

Translation: pos-14.0/pos-14.0-pos_session_pay_invoice
Translate-URL: https://translation.odoo-community.org/projects/pos-14-0/pos-14-0-pos_session_pay_invoice/it/
@carlos-lopez-tecnativa
[IMP] pos_session_pay_invoice: pre-commit stuff
9102832
@legalsylvain
Copy link
Contributor

Thanks !

@legalsylvain
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1226-by-legalsylvain-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 19, 2024
@OCA-git-bot
Merge PR #1226 into 16.0
05514ec 
Signed-off-by legalsylvain
@carlos-lopez-tecnativa
Copy link
Contributor Author

I have no idea why the test failed. It works fine on my local environment. Does anyone have any ideas? The test that failed is in other modules.

image

image

@OCA-git-bot
Copy link
Contributor

@legalsylvain your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1226-by-legalsylvain-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@carlos-lopez-tecnativa
Copy link
Contributor Author

I can see the error is general in the tour; other PRs have the same error in some OCA repos

https://github.com/OCA/calendar/actions/runs/10941425935/job/30376034658#step:8:159

https://github.com/OCA/pos/actions/runs/10921083459/job/30312371146#step:8:1086

https://github.com/OCA/e-commerce/actions/runs/10902558739/job/30254730888#step:8:306

This was referenced Sep 19, 2024
Closed
Closed
@carlos-lopez-tecnativa
Copy link
Contributor Author

@legalsylvain @pedrobaeza the PR is ready. The error in the tests is resolved in OCA/oca-ci#78.

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1226-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9394890 into OCA:16.0 Sep 25, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 16.0-mig-pos_session_pay_invoice branch September 25, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legalsylvain legalsylvain legalsylvain left review comments

@ivantodorovich ivantodorovich ivantodorovich approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.