• Nicolas Wavrant's avatar
    Verify parameters passed to category setters · 3978dba1
    Nicolas Wavrant authored
    This merge request aims to prevent programming errors by raising instead of silently doing nothing (which is a source of bugs).
    
    Currently, if an object as category property for a category "category", 2 families of setters were created :
    
    1. The string setter, following the format ```_setCategory``` and taking a relative URL as the argument.
    2. The value setter, following the format ```_setCategoryValue``` and taking an object as the argument.
    
    The issue is that developers may pass the wrong argument to one of these functions, having for consequences :
    
    1. For case (1), if an object is passed, the code would silently do nothing : nothing is set as relation, but the code doesn't fail. This is the worst case. 
    2. For the second case, passing a relative URL to ```_setCategoryValue``` would "work" (in the meaning the relation is set to the correct object). This may sound like a feature, but in my opinion it is confusing given the way ERP5 developers apprehend these setters nowadays.
    
    For case 1, a test is existing that an exception is raised, but due to coding error the feature disappeared and no one noticed : https://lab.nexedi.com/Nicolas/erp5/blob/b3ed2210ed6b30390b901c8620de6eafcc27a574/product/CMFCategory/tests/testCMFCategory.py#L810-815
    
    For case 2, compatibility code exists in the underlying function ```_setValue``` : https://lab.nexedi.com/Nicolas/erp5/blob/b3ed2210ed6b30390b901c8620de6eafcc27a574/product/ERP5Type/Base.py#L1840-1842
    
    In this MR, the exception caused by case (1) has been restored (so now it fails loudly).
    
    Case (2) has been deprecated, in order to keep backward compatibility.
    
    I have run the tests with the ```DeprecationWarning``` raising an error instead of just a warning,  and fixed the code were the setters weren't used correctly.
    
    Ideally, tests should always run with this ```DeprecationWarning``` being a real error so both cases crash loudly. This won't be part of this Merge Request.
    
    
    /reviewed-on nexedi/erp5!938
    3978dba1
Base.py 144 KB