frappe / erpnext

Free and Open Source Enterprise Resource Planning (ERP)
https://erpnext.com
GNU General Public License v3.0
20.69k stars 7.12k forks source link

Unable to create item variant with v9.2.24 and v10 #12185

Closed sathishpy closed 6 years ago

sathishpy commented 6 years ago

I have a item template with GSM attribute having integer value with range 100-250(increment 5). Trying to create a variant of gsm attribute 120 results in 'Value 120 for Attribute GSM does not exist in the list of valid Item Attribute Values for Item PPR-BRW-16-120-90' This used to work fine in earlier releases.

sathishpy commented 6 years ago

Just started looking at the source, looks like something to do with the changes from @nabinhait. Will continue to investigate further.

sathishpy commented 6 years ago

The original code in get_attribute_values(): for t in frappe.get_all('Item Attribute', fields=["name", "from_range", "to_range", "increment"], filters={'numeric_values': 1}): numeric_values[t.name.lower()] = t is changed to for t in frappe.get_all('Item Variant Attribute', fields=["attribute", "from_range", "to_range", "increment"], filters={'numeric_values': 1, 'parent': item.variant_of}): numeric_values[t.attribute.lower()] = t It looks like the attempt is to get only the attribute list that belongs to a particular template. However this will not work as the earlier 'Item Variant Attribute' doesn't contain the full details of 'Item Attribute'. In my case numeric_values, from_range, to_range all are set to 0

MariaDB [c31ef9ed1c207423]> select attribute, from_range, to_range from tabItem Variant Attribute where parent='PPR'; +-----------+------------+----------+ | attribute | from_range | to_range | +-----------+------------+----------+ | Deck | 0.000000 | 0.000000 | | BF | 0.000000 | 0.000000 | | GSM | 0.000000 | 0.000000 | | Colour | 0.000000 | 0.000000 | +-----------+------------+----------+ 4 rows in set (0.01 sec)

MariaDB [c31ef9ed1c207423]> select attribute, from_range, to_range from tabItem Variant Attribute where parent='PPR' and numeric_values=1; Empty set (0.00 sec)

which means the logic needs to combine both the tables to filter attributes belonging to a specific item variant.

nabinhait commented 6 years ago

@sathishpy Thanks for reporting. I will write a patch to set the ranges in Item Variant Attribute if blank.

sathishpy commented 6 years ago

Ideally, you don't need from_range, to_range, increment, numeric_values in 'tabItem Variant Attribute'. They are the fields of 'tabItem Attribute'. We already have 'attribute' in 'tabItem Variant Attributeand it is better to extract from_range, to_range, increment_value fromtabItem Attributerather thantabItem Variant Attribute`. That way, we could fix the get_attribute_values(): function instead of adding a new patch. This gives us the provision to cleanup 'tabItem Variant Attribute' in future when time permits

nabinhait commented 6 years ago

That's the way I also thought it should behave. But unfortunately many users modify the range after fetching it in Item Variant Attribute table and they expect the validation should be based on it. They set a wide range in the "Item Attribute" and later set a more relevant range in the template.

sathishpy commented 6 years ago

That doesn't sound right. In item, we could only set the attribute and its value. I don't see any way of changing the range in item or template itself. Attribute includes the ranges as well. So setting different ranges means, it should be a different attribute.

nabinhait commented 6 years ago

@adityaduggal Can you give some input here?

adityaduggal commented 6 years ago

@nabinhait @sathishpy Please note that there is definitely a need for from_range and to_range in the Item Template. This is how it is helpful: We have a item attribute = d1_mm, now we have defined the range of this attribute in Attribute as 1 to 1000 with increments of 0.001 But since we have many different templates using the same attribute where the range is different from the one defined in the attribute we use this feature in all the templates.

So template1 might have from range 10 to 100 with step 10 whereas template2 would have range defined for d1_mm as 900 to 1000 with step 5,

Please note that the logic is that the template range always remains the SUBSET of the range defined in the Attribute.

So if we try and make a template with range 1000 to 2000 with step 100 this would not be possible since the range defined in the attribute is 1 to 1000.

@sathishpy I think the logic is GOOD and covers all scenarios and since ERPNext is for all I think a valid rule to cover all use cases is better.

@nabinhait I think the logic can be made better by ensuring that if the FROM and TO range in the template is blank then it goes back to the attribute and use its range other wise the template range should be used which SHOULD BE A SUBSET OF THE ATTRIBUTE RANGE.

sathishpy commented 6 years ago

@adityaduggal, I think you have over complicated the usecase with no additional value. As per you requirement, you need template 1 with attribute = d1_t1_mm (range 10-100 step 10) template 2 with attribute = d1_t2_mm (range 900-100 step 5) template 3 with attribute = d1_t3_mm (range 200-800 step 50) and so on I don't see any value by creating an additional superset attribute called d1_mm covering all the ranges. Even if there is , we are looking at creating an attribute template. Keeping attribute ranges in the item variant is against the principles of database modelling.

sathishpy commented 6 years ago

To put this in another way, you will have greater control by creating three different attributes with specific ranges than changing the range in the item variant itself. Item variant should only have the attribute value, not the range. Range definition is part of the attribute, which means you are creating a new attribute itself.

adityaduggal commented 6 years ago

@sathishpy We are in the business of manufacturing cutting tools where in we have around 150+ templates and each template has around 10~20 attributes with numerical ranges and going by your suggestion I would have to create close to 1000 attributes to define close to 15k items which seems like defeating the purpose of the attributes so your suggestion does not apply to my use case.

sathishpy commented 6 years ago

Right. If you have 150 templates with ~10 attributes per template, you will have ~1500 unique attributes, so they need to be created manually. The reason for creating them manually is because they are unique(with unique ranges) Lets contrast this with your approach of having a master attribute and creating dependent attributes in the template. Since you have 150 templates with ~10 attributes per template, you will end up redefining the attribute range in all those template attributes. That again comes to 1500, same as the first approach. What did I miss here in your usecase? Apart from that, I don't see any way of specifying the attribute range either in template of item. So I really don't know how to get your suggestion implemented without changing the database directly.

adityaduggal commented 6 years ago

the most import thing you miss in this is that if we have 1500 attributes then we would have 1500 columns in our reports as well which is currently limited to 10~15 only since we are reusing the attributes.

Check the Item Report in https://github.com/adityaduggal/rigpl-erpnext/tree/master/rigpl_erpnext/rigpl_erpnext/report/item_report

Now in this report there if we have had templates as per your suggestion then this report would have had 1000s of columns and making the report would have become an exercise which is futile to say the least.

image

In the above report there are only 3 attributes which are reusing the range in various templates and hence its easier to read and search but if we had your suggestion in the erpnext then this would have meant around 20 attributes making the searching of items almost impossible at our end.

I think you should close this issue since your use case is covered if you define the range in the attribute and NOT in the TEMPLATE and that case I think is already covered so for the benefit of all users this is a better model than the one you are suggesting since that would not cover our use case.

nabinhait commented 6 years ago

I personally don't see any problem changing ranges in Item master. If it changed, it will consider the updated value, otherwise it is same as Attribute master. I will surely make one change, to ensure that if the FROM and TO range in the template is blank then it goes back to the attribute.

sathishpy commented 6 years ago

The reason for opening the issue and spending time on it is not just to solve my usecase, but to check what is better for ERPNext as a product. Though, the discussion is centered on your case, it would be a futile exercise if we limit it to solve just our current problem.

Check the Item Report in

I have made similar custom report as well for items with attributes. But it is impractical to have a general item report at ERPNext product level listing all the attributes as columns as you have rightly reported there could be thousands of attributes

In the above report there are only 3 attributes which are reusing the range in various templates

I don't think it is difficult to have the same report using my approach. Here is how I would do it. The item template will have attribute name and attribute values. We are creating multiple attributes mainly to have different ranges. So I would group the attributes based on their type(like d1_t1_mm, d1_t2_mm, d1_t3_mm grouped as d1_mm).

I personally don't see any problem changing ranges in Item master

The main question I had was on this item. How can one change the range in the item master? I checked my template which only allows changing the value. There is no way to change the range. I didn't get any answer so far as how this was done. I assume it was done by updating the database directly. If that is the case, this is a regression introduced to support a usecase that is not part of ERPNext!

I personally don't see any problem changing ranges in Item master

I agree. I wouldn't see a bigger problem if it was part of the product feature. At worst, we are flooding the database with unnecessary values, which could have been optimized. I would worry about it only when it results in noticeable performance degradation...

I will surely make one change, to ensure that if the FROM and TO range in the template is blank then it goes back to the attribute.

That is really kind of you. Since no one else has shouted about this so far, we could close this discussion.