setAD_User_ID() breaks when logged in as System user

Description

I just found that I can not do some things when logged in as System.

Whenever I try do use a process that involves saving a record with an AD_User_ID field
I get an UI error:
Could not save changes

together with this a console output:
(...).saveError: FillMandatory - AD_User_ID [9477]

Steps to reproduce:

  1. Log in as System

  2. Open any window (I tried with client window)

  3. Switch to grid view

  4. Click the customize toolbar button

  5. click OK in the dialog

Proposed solution:
Add overridden version of setAD_User_ID() to class MTabCustomization:
(Note: I have not tested the solution. Yet I am pretty confident it will fix the issue)

Environment

None

Activity

Show:
Andreas Sumerauer
July 27, 2020, 1:43 PM
Edited

I have just tested the proposed fix and can confirm that it works.
I have to add though that IMO the real problem is with the blueprint of the generated Model classes (X_WhatEver)
Due to code duplication introduced through the model generator the faulty setAD_User_ID() method is present in many of the generated model classes.
A more sustainable solution should modify the blueprint code instead of just overriding it in some places.
One might even consider to introduce a base class that provides this and other methods in order to avoid code duplication.

Andreas Sumerauer
July 27, 2020, 5:59 PM

I have had a look at Idempiere-2339 And I have to say I couldn’t agree less. To me the solution to leave the System user AD_User_ID inconsistency in place is just another quirk that every new developer will sooner or later bump into and waste some time to understand and get accustomed to. All these little quirks and ‘known annoying conditions' make the software unaccessible for developers new to the API. Also the assumed positive effect is rather limited. After all there is also a SuperUser with an AD_User_ID of 100 who is not at all effected by either 2339 (or 4386). Here the assumed safeguard is not in place and that does not seem to be an issue at all.
In my own software projects there were a number of instances where I found that a complicated feature that I had trouble writing the user documentation for could just be removed without barely any loss for the product. I am pretty sure that the developer API would be better and a liittle bit more conchise if that exception for the System user were removed and not just worked around.

Carlos Ruiz
July 27, 2020, 6:43 PM

Thanks for your opinion. I'm very aware of these kind of issues on software maintenance, and you can be sure that if we haven't solved the issue is not because of lazyness, or because we like to see design flaws

The thing is from time to time we find some of these strange design decisions that Jorg Janke made on compiere, and although solution seems pretty easy, iDempiere is too big as well as the effect that some changes can cause in all the plugins out there. So, in a few cases we go the safe route of just leaving that strange design as it is and use the workaround that compiere also designed.

There are several potential solutions I see:
1 - ask Jorg Janke why he wrote it that way I think he won't answer happily
2 - review deeply the code to check the consequences of such change, and also try to foresee the consequences on plugins
3 - do the change and try to test the whole system
4 - do the change and cross fingers
5 - try to minimize the uncertainty having more automated tests

We're working towards the 5th - and everybody is invited to help with this initiative contributing unit tests.
Or if somebody else can have the time to contribute full end-to-end tests, and deep impact analysis covering potential issues with plugins.

Carlos Ruiz
July 27, 2020, 7:03 PM

BTW - when I think about this (revisited from time to time, like in 2014) - I tend to think that JJ wrote it this way specifically to avoid people saving by mistake records in System user, so, if you think about it, is very easy for a developer of a plugin for example, to forget setting the value for a user variable and ending with a document record made by System user, or a request pointing to System, or all the variations.

So, doing it this strange way, JJ enforced the developer to think specifically when a record can contain System user reference and write the code accordingly, so, the default is that most system tables do not accept (and they don't need) reference to System user, and the few cases where we want that we write the workaround.

Again, is just my opinion trying to figure what drove JJ to design it this way - the code is clearly intentional when you see the class ModelGenerator.createColumnMethods -> "// check special column"

Andreas Sumerauer
July 27, 2020, 9:34 PM

Thanks, your view really makes more sense to me now after looking at the code example you provided.
Still I don’t think it is an appropriate api design. As I already said there is a SuperUser that should be treated in the same way as the System user and the existing design won’t do that. Also System user can create any number of users on the System side that will have a non zero user ID.
Also if it weren’t for the System user you would just end with a null pointer exception if you forgot to initialize the user ID. To the contrary the existing Api quirk will even cover up any missing initialization error wherever the system user fix is in place.

That said I do understand your position. Fixing this in a straightforward manner could potentially have negative side effects that could break the application or a third party plugin in a running system and I would rather not think of the consequences. So I am more than happy that I am not the one that has to make these decisions. I gladly leave that to you and instead enjoy my privilege of beeing able to think out loud without thinking too much of the implications. I just hope you find my ideas useful.


Assignee

Carlos Ruiz

Reporter

Andreas Sumerauer

Labels

None

Tested By

None

Components

Fix versions

Affects versions

Priority

Major
Configure