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

10.0 contract #40

Merged
merged 7 commits into from
Dec 5, 2016
Merged

10.0 contract #40

merged 7 commits into from
Dec 5, 2016

Conversation

StefanRijnhart
Copy link
Member

No description provided.

@StefanRijnhart StefanRijnhart added this to the 10.0 milestone Nov 12, 2016
@pedrobaeza
Copy link
Member

Travis is failing saying that decimal_precision is not imported. Maybe dependency chain has changed? Can you check it?

@pedrobaeza pedrobaeza mentioned this pull request Nov 12, 2016
Closed
8 tasks
@StefanRijnhart
Copy link
Member Author

No worries @pedrobaeza. That is why it's work in progress...

@StefanRijnhart StefanRijnhart force-pushed the 10.0-contract branch 3 times, most recently from 096aaef to 3a6d3e9 Compare November 13, 2016 11:48
@StefanRijnhart
Copy link
Member Author

Now ready for review. Because the 10.0 branch had not received any changes I simply merged in the latest changes of 9.0. I hope that is alright.
Only substantial change is that the sale groups are now no longer available anymore without the sale module, so I transferred the permissions to the accounting groups. Right now, the accounting user groups can only read the analytic invoice line configuration, but maybe they should have all permissions instead?

susport
susport reviewed Nov 17, 2016
View reviewed changes
contract/README.rst
@@ -21,16 +22,27 @@ To use this module, you need to:

#. Go to Sales -> Contracts and select or create a new contract.
Copy link

@susport susport Nov 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Contracts menu is not in Sales, it is in Invoicing menu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I have realigned this description with reality.

susport
susport reviewed Nov 17, 2016
View reviewed changes
Copy link

@susport susport left a comment

Choose a reason for hiding this comment

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

Validate that Date of Next Invoice is greater than the current date.

@StefanRijnhart
Copy link
Member Author

@susport I think being able to invoice retroactively could be useful in some cases so I would rather not implement the check you suggested.

pedrobaeza
pedrobaeza reviewed Nov 21, 2016
View reviewed changes
contract/__manifest__.py
@@ -5,19 +5,19 @@

{
'name': 'Contracts Management recurring',
'version': '9.0.1.0.0',
'version': '10.0.1.1.0',
Copy link
Member

Choose a reason for hiding this comment

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

10.0.1.0.0, as the first 10.0 version

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

pedrobaeza
pedrobaeza reviewed Nov 21, 2016
View reviewed changes
contract/models/contract.py
@@ -145,11 +155,34 @@ def _onchange_recurring_invoices(self):
self.recurring_next_date = self.date_start

@api.model
def get_relalive_delta(self, recurring_rule_type, interval):
Copy link
Member

Choose a reason for hiding this comment

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

s/relalive/relative

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

pedrobaeza
pedrobaeza reviewed Nov 21, 2016
View reviewed changes
contract/models/contract.py
})
# Get other invoice values from partner onchange
invoice._onchange_partner_id()
return invoice._convert_to_write(invoice._cache)
return invoice._convert_to_write(
{name: invoice[name] for name in invoice._cache})
Copy link
Member

Choose a reason for hiding this comment

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

Why changing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because in the case of empty values for many2one records, the cache contains an empty tuple instead of an empty recordset. The invoice record contains the correct value.

Before I added this change, this line broke the test. I took the modification from the Odoo core addons where it is used in several places.

Copy link
Member

Choose a reason for hiding this comment

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

Uhm, that's something ugly that doesn't happen on v9. Another hack to take into account...

lasley
lasley approved these changes Nov 30, 2016
View reviewed changes
Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

LGTM thanks @StefanRijnhart

@pedrobaeza
Copy link
Member

@StefanRijnhart, can you please squash last 4 commits into one before proceeding to merge?

@StefanRijnhart
Copy link
Member Author

Thanks for the reviews. Discovered a bug though.

@pedrobaeza about the squashing, that would remove credits so I'd rather not.

@StefanRijnhart
Copy link
Member Author

@pedrobaeza Oh, the last four commits. Will do!

@pedrobaeza
Copy link
Member

No, what I want you to do is to squash your migration commits together.

@pedrobaeza
Copy link
Member

We have commented at the same time 😉

@lasley lasley mentioned this pull request Nov 30, 2016
Merged
1 task
lasley
lasley suggested changes Dec 1, 2016
View reviewed changes
Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

contract_variable_quantity is included here, but not installable.

@lasley lasley mentioned this pull request Dec 1, 2016
Merged
1 task
@StefanRijnhart
Copy link
Member Author

Ready for merging, rebased and all. The bug I experienced was related to another call to convert_to_write. However, the problem with this method was fixed upstream in odoo/odoo@51680426, so I could revert my changes to this call.

@pedrobaeza
Copy link
Member

I'm relief to see that we don't need to introduce a new hack.

@pedrobaeza
Copy link
Member

Merging this PR. @lasley, please rebase for your PR about variable_quantity.

@pedrobaeza pedrobaeza merged commit 43636b6 into OCA:10.0 Dec 5, 2016
@lasley lasley mentioned this pull request Apr 3, 2017
Merged
@rafaelbn
Copy link
Member

rafaelbn commented May 6, 2017

I see some changes in the menus, are they intentionally?

2017-05-07_0-55-32

2017-05-07_1-01-53

@StefanRijnhart
Copy link
Member Author

@rafaelbn Yes, sale menus are no longer available in Odoo 10.0 without the additional dependency on the sale module so I moved the menus to the accounting menu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susport susport susport left review comments

@lasley lasley lasley requested changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@StefanRijnhart @pedrobaeza @rafaelbn @lasley @susport @carlosdauden @oca-transbot @OCA-git-bot