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][MIG] sale_start_end_dates #2327

Closed
wants to merge 32 commits into from

Conversation

vancouver29
Copy link
Contributor

No description provided.

@vancouver29 vancouver29 force-pushed the 16.0-mig-sale_start_end_dates branch 13 times, most recently from 3c1488f to f3bb874 Compare January 6, 2023 14:53
@rousseldenis
Copy link
Contributor

/ocabot migration sale_start_end_dates

@rousseldenis
Copy link
Contributor

@vancouver29 Thanks for this. Could you follow migration guide in order to have two commits (one for pre-commit, one for migration). https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-16.0

@@ -96,7 +96,7 @@ def _inverse_number_of_days(self):
number_of_days=line.number_of_days,
)
line.number_of_days = 1
if line.start_date:
if line.start_date and not line.end_date:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the end date converted into computed field readonly False ?

@@ -160,12 +160,12 @@ def start_end_dates_product_id_change(self):
if self.product_id.must_have_dates:
if self.order_id.default_start_date:
self.start_date = self.order_id.default_start_date
else:
self.start_date = False
# else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove unused code

@rousseldenis
Copy link
Contributor

@vancouver29 Could you also convert your PR title to human readable one ? Like '[16.0][MIG] sale_start_end_dates'. Thanks

@vancouver29 vancouver29 force-pushed the 16.0-mig-sale_start_end_dates branch from f3bb874 to 556ac39 Compare January 11, 2023 11:42
@vancouver29 vancouver29 changed the title 16.0 mig sale start end dates [16.0][MIG] sale_start_end_dates Jan 11, 2023
@vancouver29
Copy link
Contributor Author

@rousseldenis FYI

Copy link
Contributor

@rousseldenis rousseldenis 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


def test_prepare_invoice_line(self):
invoice_line_vals = self.so.order_line._prepare_invoice_line()
self.assertEqual(invoice_line_vals["product_id"], self.product_id.id)

Choose a reason for hiding this comment

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

we can add two more lines here to check start_date and end_date on invoice line

self.assertEqual(invoice_line_vals["start_date"], self.so.order_line[0].start_date)
self.assertEqual(invoice_line_vals["end_date"], self.so.order_line[0].end_date)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chandni299 i added these lines

@vancouver29 vancouver29 force-pushed the 16.0-mig-sale_start_end_dates branch from 556ac39 to 4842535 Compare January 17, 2023 11:45
Copy link

@chandni299 chandni299 left a comment

Choose a reason for hiding this comment

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

Looks Good 👍

@DorianMAG
Copy link
Contributor

Hi, looks good to me.
Tested on local environment

@DorianMAG
Copy link
Contributor

Maybe, can you rebase your branch with main branch?
Regards

@alexis-via
Copy link
Contributor

I improved this PR by converting onchange to compute methods, cf this other PR #2577
So I propose to close this PR.

@rousseldenis
Copy link
Contributor

@vancouver29 Do you mind if we switch to #2577 ?

@alexis-via
Copy link
Contributor

The module has been merged in v16, we can now close this PR

@dreispt dreispt closed this May 2, 2024
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.