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

[11.0] [MIG] sale/purchase_procurement_analytic > procurement_mto_analytic #204

Merged
merged 10 commits into from
Nov 20, 2018
Merged

[11.0] [MIG] sale/purchase_procurement_analytic > procurement_mto_analytic #204

merged 10 commits into from
Nov 20, 2018

Conversation

gittusr
Copy link

@gittusr gittusr commented Nov 14, 2018

No description provided.

@oca-clabot

This comment has been minimized.

Sign in to view
@pedrobaeza pedrobaeza added this to the 11.0 milestone Nov 14, 2018
This was referenced Nov 14, 2018
Closed
Closed
@aisopuro
Copy link

Hi @gittusr . I'm not extremely familiar with this module construction, but based on the READMEs in the 10.0 branch, it seems like the original way these modules worked was that you had a "core module" (procurement_analytic) which allowed you to have an analytic account as part of the procurement chain. Other modules could then hook into this logic as they wanted: sale_procurement_analytic could add the sale's analytic account to the procurement, and purchase_procurement_analytic could read it if it was present.

The procurement logic has changed a lot from 10.0 to 11.0, and especially the procurement.order model has disappeared, so there isn't really a specific model designed to handle procurements anymore.

I think it would be good if you could explicitly mention this logic change and the reasons why you removed all the other modules in the commit message so that there is an explanation in the history of why the modules disappeared.

I also don't know what the policy is, but since this "new" module combines sale and purchase functionality, I feel like it should perhaps have a new name. sale_purchase_procurement_analytic perhaps?

aisopuro
aisopuro approved these changes Nov 15, 2018
View reviewed changes
Copy link

@aisopuro aisopuro left a comment

Choose a reason for hiding this comment

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

Look good to me. 👌

AaronHForgeFlow
AaronHForgeFlow approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Functional test 👍

As there is an object called procurement rule it is ok to me to keep the module name.

pedrobaeza
pedrobaeza requested changes Nov 15, 2018
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 agree with @aisopuro that the scope has changed and thus, you have to rename the module name. Basically, as I understand, this one gets rid out the previous 3 modules: procurement_analytic, sale_procurement_analytic and purchase_procurement_analytic.

So for me the most reasonable option is to let the name of this in just procurement_analytic, or go a bit further with procurement_mto_analytic, because at the end, this is used for MTO only.

pedrobaeza
pedrobaeza approved these changes Nov 15, 2018
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.

Squashing migration commits (and maybe you can generate the README already following https://github.com/OCA/maintainer-tools/#readme-generator

@gurneyalex
Copy link
Member

I'm closing and reopening to test the clabot.

@gurneyalex gurneyalex closed this Nov 18, 2018
@gurneyalex gurneyalex reopened this Nov 18, 2018
@gurneyalex gurneyalex mentioned this pull request Nov 18, 2018
Merged
@okuryan
Copy link

okuryan commented Nov 20, 2018

@pedrobaeza , we see that Pull request is marked as approved, but still see that there are some issues with it. Can @gittusr help somehow? Any actions required from us?

@pedrobaeza pedrobaeza changed the title [11.0] [MIG] purchase_procurement_analytic [11.0] [MIG] sale/purchase_procurement_analytic > procurement_mto_analytic Nov 20, 2018
carlosdauden and others added 10 commits November 20, 2018 09:47
@carlosdauden @pedrobaeza
[NEW] purchase_procurement_analytic: New module
25e3c8b
@cubells @pedrobaeza
[MIG] purchase_procurement_analytic module: migration to 10.0
a099058
@oca-transbot @pedrobaeza
OCA Transbot updated translations from Transifex
f06fb32
@rousseldenis @pedrobaeza
[10.0][FIX] purchase_procurement_analytic: Really group by analytic a…
b8aa69e 
…ccount

The search on purchase creation from procurement is done on purchase.order model
Don't use a context change to do so
@oca-travis @pedrobaeza
[UPD] Update purchase_procurement_analytic.pot
5c24ef8
@rousseldenis @pedrobaeza
[10.0][FIX] purchase_procurement_analytic: Purchase grouping
c5417d9 
When procurements with no analytic account (e.g. Reordering rules) are run with
existing purchase orders (with analytic account defined), it adds purchase line
on purchase with order lines with analytic. It shouldn't.
@cubells @pedrobaeza
[FIX] *procurement_analytic: duplicated account analytic field in view
58f9597
@rschnapka @pedrobaeza
Translated using Weblate (German)
f53a95c 
Currently translated at 100,0% (2 of 2 strings)

Translation: account-analytic-10.0/account-analytic-10.0-purchase_procurement_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-10-0/account-analytic-10-0-purchase_procurement_analytic/de/
@pedrobaeza
Translated using Weblate (Catalan)
89616f8 
Currently translated at 100,0% (2 of 2 strings)

Translation: account-analytic-10.0/account-analytic-10.0-purchase_procurement_analytic
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-10-0/account-analytic-10-0-purchase_procurement_analytic/ca/
@pedrobaeza
[MIG] purchase_procurement_analytic > procurement_mto_analytic: Migra…
3138a6a 
…tion and adaptation to 11.0
@pedrobaeza pedrobaeza force-pushed the 11.0-mig-purchase_procurement_analytic branch from 11a32af to 3138a6a Compare November 20, 2018 09:03
@pedrobaeza
Copy link
Member

I have rebased the branch and did following things:

  • Restore authorship and contributors. @gittusr you can't remove our authorship from the module. I have respected yours as you have touched enough the module for granting it though.
  • Generated README.rst from fragments.
  • Correct URL in manifest.
  • Squashed commits.

If everything goes right, I'll merge.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza pedrobaeza merged commit bcdc470 into OCA:11.0 Nov 20, 2018
@MiquelRForgeFlow MiquelRForgeFlow mentioned this pull request Aug 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aisopuro aisopuro aisopuro approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

@AaronHForgeFlow AaronHForgeFlow AaronHForgeFlow approved these changes

Assignees
No one assigned
Labels
approved needs review ready to merge
Projects
None yet
Milestone
11.0
Development

Successfully merging this pull request may close these issues.

None yet