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

[ADD] product_unique_code. #34

Closed
wants to merge 3 commits into from

Conversation

@LeartS
Copy link
Contributor

commented Dec 18, 2014

No description provided.

'category': 'Product',
'description': """
Product Unique Codes
===========================

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Dec 18, 2014

Member

Put same length of the title.

@coveralls

This comment has been minimized.

Copy link

commented Dec 18, 2014

Coverage Status

Coverage increased (+4.89%) when pulling e37c458 on LeartS:8.0-product_unique_code into 1702be8 on OCA:8.0.

@coveralls

This comment has been minimized.

Copy link

commented Dec 18, 2014

Coverage Status

Coverage increased (+4.89%) when pulling 0ac26ca on LeartS:8.0-product_unique_code into 1702be8 on OCA:8.0.

@LeartS

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2014

I'm actually surprised the build passes, considering there are products with duplicate default_code in the demo data and the module gives error during installation when that's the case, because it can't create the unique constraint.

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Dec 18, 2014

That error is bypassed by the Travis checks.


@api.one
def copy(self, default=None):
new_code = self.default_code + '-2'

This comment has been minimized.

Copy link
@gurneyalex

gurneyalex Dec 18, 2014

Member

this will fail if you try to copy the same product twice as you will end up with 2 products with dfefault_code = 'orig_code-2'

This comment has been minimized.

Copy link
@LeartS

LeartS Dec 18, 2014

Author Contributor

Yeah well, it will also fail if the user already has, for whatever reason, another product with default code orig_code-2.
Finding a pseudo-random "free" default_code is imo overkill, the user should edit the code anyway, -2 is just to show a default value. The same way I don't think users leave (Copy) in the product name when they duplicate it.

Maybe it's better to add a specific error that tells them to change the code before duplicating the product again?

This comment has been minimized.

Copy link
@pedrobaeza

pedrobaeza Dec 18, 2014

Member

I will prefer to put field as not required, and with attribute copy=False to not propagate previous value. Constraint doesn't apply to duplicated null values.

This comment has been minimized.

Copy link
@gurneyalex

gurneyalex Dec 18, 2014

Member

+1 for @pedrobaza's solution

This comment has been minimized.

Copy link
@LeartS

LeartS Dec 18, 2014

Author Contributor

@pedrobaeza the unique constraint doesn't apply to null values at all, at least it seems from my tests.
But I've tested leaving required=True with copy=False (which I didn't know about, thanks!) and it seems like it works: it creates and opens in edit mode the duplicate, but doesn't let you save without specifying a code (but it lets you discard, in which case the duplicated product will have null code o,o)

So, copy=False works with both required=True or required=False. What's your opinion on this? Should unique code imply required code? Also keep in mind that with required=True the code field is highlighted, which might hint the user he should change the field.

@gurneyalex

This comment has been minimized.

Copy link
Member

commented Dec 18, 2014

@LeartS you need to patch the demo data in the demo data of your addon to fix the default code.

@coveralls

This comment has been minimized.

Copy link

commented Dec 19, 2014

Coverage Status

Coverage increased (+4.65%) when pulling 8d6b7a3 on LeartS:8.0-product_unique_code into 1702be8 on OCA:8.0.

@sebastienbeau

This comment has been minimized.

Copy link
Member

commented Dec 19, 2014

Hi I think it will be better to merge this previous merge here : #33
Which is a basic extraction of the unique_code feature in the product sequence module.
And than port this merge into 8 version and change the API.
Please also keep the copyright in the header ;)

@LeartS

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2014

The module is so small that I don't see any difference between writing it from scratch or "porting it" from v7, and surely faster than wait for another PR request to be reviewed and accepted.

Regarding the header, are you talking about the product.py file having no license/copyright notices at the start? Is there an OCA conventions on having those on every single .py file? I think having it on the openerp.py file is already more than enough

@yvaucher

This comment has been minimized.

Copy link
Member

commented Dec 19, 2014

@LeartS I cannot remind of a talk about a convention on this

However, from what I can read on http://www.gnu.org/licenses/agpl-3.0.html

To do so, attach the following notices to the program. It is safest to attach them
to the start of each source file to most effectively state the exclusion of warranty;
and each file should have at least the "copyright" line and a pointer to where the full notice is found

You would at least something like that

# Copyright 2014 <author>
# <notice where to find full licence>

And this won't harm

# -*- coding: utf-8 -*-

Maybe it's time to talk about license on community mailing list?

@LeartS

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2014

Maybe it's time to talk about license on community mailing list?

I think it's something that should be discussed. Can you start a thread about it?

@clonedagain

This comment has been minimized.

Copy link

commented Jan 5, 2015

👎
This is at least the third proposal for the same basic module here :

And the same feature exists also in the project "multi-company", only making it company-aware :

Please stop, comment on existing proposals and let's converge.

@yvaucher

This comment has been minimized.

Copy link
Member

commented Jan 5, 2015

@LeartS LeartS closed this Aug 21, 2015

@yvaucher yvaucher removed the needs fixing label Aug 21, 2015

@LeartS LeartS deleted the LeartS:8.0-product_unique_code branch Oct 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.