Skip to content

[14.0][ADD] product_attribute_value_price_import module#1079

Merged
OCA-git-bot merged 2 commits into
OCA:14.0from
ilyasProgrammer:14.0-product_attribute_value_price_import
Jun 14, 2022
Merged

[14.0][ADD] product_attribute_value_price_import module#1079
OCA-git-bot merged 2 commits into
OCA:14.0from
ilyasProgrammer:14.0-product_attribute_value_price_import

Conversation

@ilyasProgrammer
Copy link
Copy Markdown
Member

@ilyasProgrammer ilyasProgrammer commented May 18, 2022

This module adds a menu item in sales > configuration where user can see/edit all extra prices set for each attribute value in each product template.

It also allows extra prices to be imported via standard csv/xlsx import.

@ilyasProgrammer ilyasProgrammer force-pushed the 14.0-product_attribute_value_price_import branch from 77ca6c8 to 1ae42a6 Compare May 19, 2022 09:35
@ilyasProgrammer ilyasProgrammer marked this pull request as ready for review May 19, 2022 09:35
@rousseldenis rousseldenis added this to the 14.0 milestone May 19, 2022
Copy link
Copy Markdown
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Functional review ok

Copy link
Copy Markdown

@olgamarcocb olgamarcocb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@OCA-git-bot
Copy link
Copy Markdown
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). 🤖

@francesco-ooops
Copy link
Copy Markdown
Contributor

@OCA/crm-sales-marketing-maintainers could you take a look?

@francesco-ooops
Copy link
Copy Markdown
Contributor

@OCA/crm-sales-marketing-maintainers any feedback? :)

@francesco-ooops
Copy link
Copy Markdown
Contributor

@OCA/crm-sales-marketing-maintainers hi, could someone take a look?

@francesco-ooops
Copy link
Copy Markdown
Contributor

ping ping @pedrobaeza @rousseldenis :)

@pedrobaeza
Copy link
Copy Markdown
Member

@francesco-ooops the rate of other PRs that you and your team review against the reviews you ask for is not balanced. You have to balance it and not ask always for the same wildcards.

@elvise
Copy link
Copy Markdown

elvise commented Jun 11, 2022

Hi @pedrobaeza thanks so much for your reply!
I’m a bit confused, in this specific case the PR has both reviews what i should do if not ping OCA ?
Please let me know.

@pedrobaeza
Copy link
Copy Markdown
Member

Please read again https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#review

One of the review must come from a PSC or maintainer.

Rules can be relaxed if the module is declared as Alpha.

Anyways, I insist that you are not reviewing any other PR except yours, so that's not fair.

@elvise
Copy link
Copy Markdown

elvise commented Jun 11, 2022

Anyways, I insist that you are not reviewing any other PR except yours, so that's not fair.

Hi @pedrobaeza
thanks a lot for your reply!
These are just some PR (that are not ours) where i’m contributing:

[ADD] 14.0 pos_product_multi_barcode by PierrickBrun · Pull Request #768 · OCA/pos

OCA/reporting-engine#591

OCA/sale-reporting#130

OCA/reporting-engine#574

OCA/sale-reporting#130

OCA/account-invoice-reporting#211

OCA/stock-logistics-reporting#177

OCA/reporting-engine#502

So now I have some questions:
how can you track my balance ?
how can you track the balance of our team ? (12 people)

I’m asking because this is really useful for us (and maybe also for others) for get the balance.

@elvise
Copy link
Copy Markdown

elvise commented Jun 11, 2022

One of the review must come from a PSC or maintainer.

That’s the reason why we need ping OCA :)

@pedrobaeza
Copy link
Copy Markdown
Member

pedrobaeza commented Jun 11, 2022

I judge it looking at the contributor profile: https://github.com/ilyasProgrammer?tab=overview&from=2022-02-01&to=2022-02-28 that there's no review there.

Thanks anyway for your involvement in other PRs. You can ping people from that PRs to review this one.

In a quick review, I would rename the module to product_attribute_value_menu and indicate in the README that this allows to import them. Anyways, why don't you import it in the "Attributes" menu entry through the one2many field?

@elvise
Copy link
Copy Markdown

elvise commented Jun 12, 2022

I judge it looking at the contributor profile: https://github.com/ilyasProgrammer?tab=overview&from=2022-02-01&to=2022-02-28 that there's no review there.

Thanks anyway for your involvement in other PRs. You can ping people from that PRs to review this one.

In a quick review, I would rename the module to product_attribute_value_menu and indicate in the README that this allows to import them. Anyways, why don't you import it in the "Attributes" menu entry through the one2many field?

Hi @pedrobaeza
and thank you for not leaving me alone in this jungle! :)

Maybe we need to fix some things in our organisation too…
So let me share to you how we organised the activities.

Developers:

  • Porting modules from previous versions
  • Backport modules from the newest versions
  • Open PRs for new modules
  • Code reviews (we need to increase it for sure)

Consultants:

  • Take care for not get the P.R. in stale (ours or not)
  • Functional review (ours or not)
  • Check if needed to squash (for save the PSC time)
  • Check for the basic checks: Runboat - Travis - Coverage Tests (for save the PSC time)
  • Translate (ours or not)
  • Stress PSC for get the final merge :)

Workflow:

  • Open task [Consultant]
  • Code [Developer]
  • Open new P.R. [Developer]
  • Functional review [Consultant]
  • Fix functional review [Developer]
  • Functional review [Consultant]
  • Code review [Developer]
  • Fix code review [Developer]

So we iterate this flow until to get:

  • Functional review > Ok
  • Code review > Ok
  • All checks > Green (Runboat, Travis, Coverage Test etc)

Now when we have these 3 points done we getting tags:

  • Approved
  • Ready to Merge

So now we expect someone from the PSC to arrive to do the final review (as this is the only way) and then merge.
For sure we understand that there is to wait as we are talking about a community one, two, three, four, five and if you want also one week, but for sure not months.
Check this case: this PR is ready for the PSC 24 days ago, but nobody is here, despite we ping constantly.
Check this case: [14.0][ADD] sale_commission_agent_restrict by ilyasProgrammer · Pull Request #354 · OCA/commission stuck from 24 days, despite we ping constantly.

I have a lot of this cases even worse than these.

Please don't take this conversation as a controversy, we are trying to figure out how to improve ourselves or how to improve the OCA flow or anyway how we can fortify the collaboration with OCA ecosystem.

For last but not least thanks again for your patience :)

@francesco-ooops
Copy link
Copy Markdown
Contributor

In a quick review, I would rename the module to product_attribute_value_menu and indicate in the README that this allows to import them. Anyways, why don't you import it in the "Attributes" menu entry through the one2many field?

Hi @pedrobaeza , I'm not sure what you are referring to, maybe the module's goal is not too clear, use case:

For product template "Configurable Desk", attribute value "Legs: steel" has an extra price of 100, "Legs: aluminum" has an extra price of 50;
for product template "Configurable Chair", attribute value "Legs: steel" has an extra price of 30, "Legs: aluminum" has an extra price of 15

in vanilla odoo you can only set these extra prices manually for each product template by clicking "configure variants", clicking on the single attribute value, edit, change price (not even bulk change, which wouldn't even be so useful); this is not convenient when importing products data for a new customer or new products.

Hence this module, which allows you to set the prices in bulk by filtering/grouping by attribute value and product template, or use the csv import for these PTAV extra price fields

in light of this, what do you think should be changed/improved?

thanks :)

@rousseldenis
Copy link
Copy Markdown
Contributor

In a quick review, I would rename the module to product_attribute_value_menu and indicate in the README that this allows to import them. Anyways, why don't you import it in the "Attributes" menu entry through the one2many field?

Hi @pedrobaeza , I'm not sure what you are referring to, maybe the module's goal is not too clear, use case:

For product template "Configurable Desk", attribute value "Legs: steel" has an extra price of 100, "Legs: aluminum" has an extra price of 50; for product template "Configurable Chair", attribute value "Legs: steel" has an extra price of 30, "Legs: aluminum" has an extra price of 15

in vanilla odoo you can only set these extra prices manually for each product template by clicking "configure variants", clicking on the single attribute value, edit, change price (not even bulk change, which wouldn't even be so useful); this is not convenient when importing products data for a new customer or new products.

Hence this module, which allows you to set the prices in bulk by filtering/grouping by attribute value and product template, or use the csv import for these PTAV extra price fields

in light of this, what do you think should be changed/improved?

thanks :)

That's a good reason to do it.

Copy link
Copy Markdown
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.

Functionnaly tested

@elvise
Copy link
Copy Markdown

elvise commented Jun 13, 2022

In a quick review, I would rename the module to product_attribute_value_menu and indicate in the README that this allows to import them

@ilyasProgrammer can you check this review ?

@francesco-ooops
Copy link
Copy Markdown
Contributor

@pedrobaeza better now?

@pedrobaeza
Copy link
Copy Markdown
Member

Yes, thanks

/ocabot merge nobump

@OCA-git-bot
Copy link
Copy Markdown
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-1079-by-pedrobaeza-bump-nobump, awaiting test results.

@pedrobaeza
Copy link
Copy Markdown
Member

Hi @pedrobaeza and thank you for not leaving me alone in this jungle! :)

Maybe we need to fix some things in our organisation too… So let me share to you how we organised the activities.

Developers:

* Porting modules from previous versions
* Backport modules from the newest versions
* Open PRs for new modules
* Code reviews (we need to increase it for sure)

Consultants:

* Take care for not get the P.R. in stale (ours or not)
* Functional review (ours or not)
* Check if needed to squash (for save the PSC time)
* Check for the basic checks: Runboat - Travis - Coverage Tests (for save the PSC time)
* Translate (ours or not)
* Stress PSC for get the final merge :)

Workflow:

* Open task [Consultant]
* Code [Developer]
* Open new P.R. [Developer]
* Functional review [Consultant]
* Fix functional review [Developer]
* Functional review [Consultant]
* Code review [Developer]
* Fix code review [Developer]

So we iterate this flow until to get:

* Functional review > Ok
* Code review > Ok
* All checks > Green (Runboat, Travis, Coverage Test etc)

Now when we have these 3 points done we getting tags:

* Approved
* Ready to Merge

So now we expect someone from the PSC to arrive to do the final review (as this is the only way) and then merge. For sure we understand that there is to wait as we are talking about a community one, two, three, four, five and if you want also one week, but for sure not months. Check this case: this PR is ready for the PSC 24 days ago, but nobody is here, despite we ping constantly. Check this case: [14.0][ADD] sale_commission_agent_restrict by ilyasProgrammer · Pull Request #354 · OCA/commission stuck from 24 days, despite we ping constantly.

I have a lot of this cases even worse than these.

Please don't take this conversation as a controversy, we are trying to figure out how to improve ourselves or how to improve the OCA flow or anyway how we can fortify the collaboration with OCA ecosystem.

For last but not least thanks again for your patience :)

About this, first of all, thanks for your involvement around OCA. I hope all the companies around working with Odoo make the same as you.

For OCA ecosystem to be sustainable, all the companies involved should balance the work to be done inside it, and this includes both contributing code (migrations, new modules, etc), with the review of the code contributed by yourself or others. And this review is both technical and functional side. Is the technical review side which I see that you lack a bit, so it's convenient that your developers also perform such reviews, first on your pull requests to assure your quality (OCA CI indicators is just the tip of the iceberg), and also on others.

It's also advisable to indicate in your reviews that it has been a functional or a code review.

Your pipeline seems good and well organized. Your next logical step is to scale your involvement requesting to be PSC of the repositories for being self-independent. This is a meritocracy, and you have to request such inclusion in contributors at odoo-community.org. You can do it in a minor scale declaring a maintainer on your new modules, so you can perform patches in a quick way without requiring a PSC.

In any case, continue with this excellent trend and hope you to see in person in Odoo experience event.

@OCA-git-bot OCA-git-bot merged commit e19edf0 into OCA:14.0 Jun 14, 2022
@OCA-git-bot
Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at baf6a3e. Thanks a lot for contributing to OCA. ❤️

@francesco-ooops
Copy link
Copy Markdown
Contributor

@pedrobaeza @rousseldenis I think in this PR a new module was merged in v.14 using the same technical name as another existing module in v.13 (product_attribute_value_menu, see #997 )

was that intentional? looks confusing to me, they also have different features and purposes

odoo store even sees them as the same module link

@pedrobaeza
Copy link
Copy Markdown
Member

It was an accidental name collapsing it seems.

@francesco-ooops
Copy link
Copy Markdown
Contributor

francesco-ooops commented Jul 27, 2022

It was an accidental name collapsing it seems.

shall we fix it somehow? maybe add a note in roadmap for both modules?

@pedrobaeza
Copy link
Copy Markdown
Member

Is the other module going to be migrated to 14? If not, we can continue this way.

@francesco-ooops
Copy link
Copy Markdown
Contributor

Is the other module going to be migrated to 14? If not, we can continue this way.

how could I predict that? 😅

and what happens if both modules need to be migrated to 15?

I think editing known issues/roadmap is the fastest way to make users aware that the one in v.13 and the one in v.14 are different modules and take necessary steps

@pedrobaeza
Copy link
Copy Markdown
Member

OK, as you prefer.

@pedrobaeza
Copy link
Copy Markdown
Member

Can't be both functionalities included in the module?

@francesco-ooops
Copy link
Copy Markdown
Contributor

@pedrobaeza doesn't sound like the worst idea :)

@JordiMForgeFlow
Copy link
Copy Markdown
Contributor

JordiMForgeFlow commented Jul 28, 2022

Hi @pedrobaeza and @francesco-ooops

We are in fact going to migrate the module (#997) to V14 and V15.

However, I think that this module you have renamed should in fact be named product_template_attribute_value_menu, as it is what it is adding.

Another option, as Pedro suggests, could be to add both functionalities in the module.

Please, let me know your thoughts so we can fix this collapse in the names :)

CC @GuillemCForgeFlow

@francesco-ooops
Copy link
Copy Markdown
Contributor

Hi @JordiMForgeFlow

I agree with both options, renaming (the name you suggest is the correct one) and "merging" both modules into one, but I would go for the latter

we can not work on this before a couple weeks, if you want to proceed please go ahead and keep us posted :)

@JordiMForgeFlow
Copy link
Copy Markdown
Contributor

Hi @francesco-ooops

Thank you for getting back :) We will take care of adding the features from the module in V13 to your module in next versions

@francesco-ooops
Copy link
Copy Markdown
Contributor

great thanks a lot!

@francesco-ooops
Copy link
Copy Markdown
Contributor

Hi @francesco-ooops

Thank you for getting back :) We will take care of adding the features from the module in V13 to your module in next versions

Hi @JordiMForgeFlow , any ETA? just for scheduling. Thanks!

@JordiMForgeFlow
Copy link
Copy Markdown
Contributor

Hi @francesco-ooops

We have now opened a PR to add the features: #1132 :)

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.

8 participants