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

[10.0][IMP] sale_commission: Add settlement lines' analysis views #216

Closed

Conversation

SimoRubi
Copy link
Member

Add menu item for analysis views:

  • graph: declared but unused
  • pivot: new view, similar to graph

@pedrobaeza pedrobaeza added this to the 10.0 milestone May 21, 2019
@pedrobaeza
Copy link
Member

Have you seen the work about analysis done in 11.0/12.0? I think it's more interesting to backport it than to create a divergence in views/features that will difficult migration path.

@SimoRubi
Copy link
Member Author

No i'm mostly using 10.0. Ok I'll take a look and backport a version for an easier migration path. 11.0 or 12.0?

@pedrobaeza
Copy link
Member

Both are the same basically. You can cherry-pick d11095d and 64dc262

@SimoRubi
Copy link
Member Author

Thanks @pedrobaeza for the hint, I tried to have a more complete set of the new commits with
git log origin/10.0..origin/11.0 --no-merges -- sale_commission
but it was producing all the commits n 11.0i because migrating creates new commits, if you have a better way to do that I'll be happy to try the backporting.

As for this PR, I cherry-picked as you suggested.

aitorbouzas and others added 2 commits May 22, 2019 12:06
@SimoRubi SimoRubi force-pushed the 10.0-imp-sale_commission-settlements_menu branch from 0c48524 to b3dff5a Compare May 22, 2019 10:07
comodel_name='res.company',
default=lambda self: self.env.user.company_id,
required=True
)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't company_id be used in a record rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

@eLBati it could be added in order to add the 'automatic filtering' in multi-company environments but, as far as I know, it is not mandatory.
In particular, I added this field because it was necessary after the changes backported from v11 and there (https://github.com/OCA/commission/tree/11.0/sale_commission) I haven't found any record rule.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add that record rules for proper multi-company support, which is indeed a pending topic.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok then I'll add them in a separate commit so we can easily forward port them

Copy link
Member

Choose a reason for hiding this comment

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

OK, great. You should put a migration scripts for emptying company_id field for avoiding mis-behaviors when you have commissions in several companies, or even better, try an heuristic for deducing the company of each settlement.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pedrobaeza emptying company_id is not possible because it is required, I'm trying to find the correct company for each settlement

Copy link
Member Author

Choose a reason for hiding this comment

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

@pedrobaeza @eLBati I added the record rules and the migration scripts as you requested, please have a look, thanks

Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing that the company is already present in 11.0:

company_id = fields.Many2one(

@SimoRubi SimoRubi force-pushed the 10.0-imp-sale_commission-settlements_menu branch 3 times, most recently from 88714d3 to 817b0e2 Compare June 10, 2019 15:57
…after backporting from 11.0

Otherwise get this:
commission/sale_commission/models/settlement.py", line 198, in _check_company
    if record.agent_line.company_id != record.company_id:
AttributeError: 'account.invoice.line.agent' object has no attribute 'company_id'
@SimoRubi SimoRubi force-pushed the 10.0-imp-sale_commission-settlements_menu branch from 817b0e2 to 12bc1f3 Compare June 10, 2019 16:24
…for settlements and their lines, and for agent lines
@SimoRubi SimoRubi force-pushed the 10.0-imp-sale_commission-settlements_menu branch from 12bc1f3 to 11688d4 Compare June 10, 2019 16:29
@SimoRubi SimoRubi force-pushed the 10.0-imp-sale_commission-settlements_menu branch from d5e9178 to eb85cff Compare June 12, 2019 10:06
Copy link
Member

@eLBati eLBati left a comment

Choose a reason for hiding this comment

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

LGTM

@SimoRubi do you want to keep the last 3 commits separated?

@SimoRubi
Copy link
Member Author

LGTM

@SimoRubi do you want to keep the last 3 commits separated?

Yes, in my opinion they refer to different improvements that might be forward ported more easily if they are separated commits

@pedrobaeza
Copy link
Member

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Rebased to ocabot-merge-pr-216-to-10.0-by-pedrobaeza-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jun 15, 2019
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

Merged at ff792d8. Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged on the main branch.

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