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

[MIG][16.0] partner_invoicing_mode_weekly (rename account_invoice_mode_weekly) #1514

Open
wants to merge 16 commits into
base: 16.0
Choose a base branch
from

Conversation

rousseldenis
Copy link
Contributor

@rousseldenis rousseldenis commented Jul 25, 2023

@rousseldenis
Copy link
Contributor Author

rousseldenis commented Jul 25, 2023

@StefanRijnhart IMHO, stock dependency should be removed in monthly module too (see second commit).

@StefanRijnhart StefanRijnhart changed the title [MIG][16.0] partner_invoicing_mode (rename account_invoice_mode_weekly) [MIG][16.0] partner_invoicing_mode_weekly (rename account_invoice_mode_weekly) Jul 25, 2023
Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Nice one!

@StefanRijnhart
Copy link
Member

/ocabot migration account_invoice_mode_weekly

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jul 25, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Jul 25, 2023
64 tasks
"""Returns the sale order fields used to group them into jobs."""
return ["partner_invoice_id", "payment_term_id"]

def _generate_invoices_by_partner(self, saleorder_ids, invoicing_mode="weekly"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@StefanRijnhart @jcoux After having read this, it's impossible to have several modes on the same instance as:

  • This is should be implemented in base module
  • This is always overridden without super in each specific module...

I'll try to fix that

@javierjcf
Copy link

Hi,
I'm testing this PR.
In my system i got an error when install due to:
https://github.com/acsone/account-invoicing/blob/b17ec89d6a1f1798010e90e0ad4e94707260fe85/partner_invoicing_mode_weekly/views/res_config_settings_views.xml#L7

I see in the history that was changed, but it does not exist in v16, i think it should be:
<div id="invoicing_settings" position="after">
like before

I also see that the function def generate_weekly_invoices(self, companies=None): was removed and now we have

return self.generate_invoices(
            companies,
            invoicing_mode="weekly",
            last_execution_field="invoicing_mode_weekly_last_execution",
        )

But generate_invoices does not exist, and i can not find it.
Is this PR Work in progress or i'm missing something? It seems a different structure compared to partner_invoicing_mode_monthly that is already merged.

@javierjcf
Copy link

javierjcf commented Aug 8, 2023

Hi, I'm testing this PR. In my system i got an error when install due to: https://github.com/acsone/account-invoicing/blob/b17ec89d6a1f1798010e90e0ad4e94707260fe85/partner_invoicing_mode_weekly/views/res_config_settings_views.xml#L7

I see in the history that was changed, but it does not exist in v16, i think it should be: <div id="invoicing_settings" position="after"> like before

I also see that the function def generate_weekly_invoices(self, companies=None): was removed and now we have

return self.generate_invoices(
            companies,
            invoicing_mode="weekly",
            last_execution_field="invoicing_mode_weekly_last_execution",
        )

But generate_invoices does not exist, and i can not find it. Is this PR Work in progress or i'm missing something? It seems a different structure compared to partner_invoicing_mode_monthly that is already merged.

I response myself: It seems we ned this other PR: #1515 in order to test this one, so function generate_invoices is on that PR.

i think is a nice refactor, but what about montlhy module that is already in 16.0 branch? It seems we need another PR if this one is merged

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-1514-by-rafaelbn-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Aug 12, 2023
Signed-off-by rafaelbn
@OCA-git-bot
Copy link
Contributor

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

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.

@rafaelbn
Copy link
Member

@rousseldenis it looks like a dependency is missing 🔴

Run for reqfile in requirements.txt test-requirements.txt ; do
odoo-addon-partner-invoicing-mode @ git+https://github.com/OCA/account-invoicing@refs/pull/1515/head#subdirectory=setup/partner_invoicing_mode
Unreleased dependencies found in test-requirements.txt.
Error: Process completed with exit code 1.

@rousseldenis
Copy link
Contributor Author

@rousseldenis it looks like a dependency is missing red_circle

Run for reqfile in requirements.txt test-requirements.txt ; do
odoo-addon-partner-invoicing-mode @ git+https://github.com/OCA/account-invoicing@refs/pull/1515/head#subdirectory=setup/partner_invoicing_mode
Unreleased dependencies found in test-requirements.txt.
Error: Process completed with exit code 1.

@rafaelbn This one is normal.

It's a warning to say we need to merge the dependency PR before this one. :-)

@rafaelbn
Copy link
Member

OK @rousseldenis ! I see! I miss it because I knew that one was migrated some time ago!

https://github.com/OCA/account-invoicing/tree/16.0/partner_invoicing_mode

So I closed your PR, could you change this one using the actual migrated? 😄

Thank you!

@rafaelbn rafaelbn self-requested a review August 16, 2023 19:05
@rousseldenis
Copy link
Contributor Author

could you change this one using the actual migrated? 😄

Already done...

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Tested, I'm unable to make this works, is it possible to test this module in runboat? See:

MIG-16-partner_invoicing_mode_weekly-1514.mp4

odooNextev pushed a commit to odooNextev/account-invoicing that referenced this pull request Sep 29, 2023
Signed-off-by moylop260
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Nov 10, 2024
@github-actions github-actions bot closed this Dec 15, 2024
@acsonefho
Copy link
Contributor

@rousseldenis Is it possible to merge this one as it's validated?

@StefanRijnhart
Copy link
Member

@rousseldenis I think test dependency can be removed.

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jan 5, 2025
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.