nexedi / erp5

Sounds fine for me.

I put checkConsistency() in validate, not in validate_action intentionally. I believe that checkConsistency() here is required in validate() itself, because we really don't want duplicate login in both user transition and logic transition.

Then we put checkConsistency() in validate_action too ? I am not sure if it is worth doing but there is a reason to do so, why not.

I think we should be consistent everywhere. Many workflows in ERP5 are doing consistency check on user action, not on workflow method doing the transition. I think this is used to separate the logic of the workflow from the technical transition themself. For any workflow, doing consistency check is important and leads to various issues if consistency checks are not done. Therefore, when scripting, we sould use workflow tool api to pass the 'validate_action', and we should not use 'validate()' directly. Having possibility to pass workflows transitions without any checking is very useful but should be rarely used. It can be required in some maintenance case, when you are doing synchronization, to repair user mistake, etc...

I understand duplicated logins are a problem, but I don't understand why login workflow would be an exception here.

Our workflow documentation says:

Assuming you make the validation yourself, or do not want the validation to occur, you can call directly the workflow method like this :( ... )

This was my case. I was writing a security test verifying that user can invalidate a login, so it looks like:

erp5_login = self.portal.person_module.newContent(portal_type='Person')\
      .newContent(portal_type='ERP5 Login')
erp5_login.validate()
self.assertUserCanPassWorkflowTransition(user, erp5_login, 'invalidate_action')

This is one valid case where we do not want validation to occur. Of course, I can modify this code to generate a unique reference and set it, but the concern is more that this workflow should comply to the rule, or we should rethink the rule.

Added 1 commit:

• c8e0b8ad - test: update tests related to login consistency checks

I agree with @kazuhiko.

Moving the validation is wrong as it puts real use cases at risk for an artificial one (which is trivial to fix: just put the user_id as a login - that value is already unique and seems unlikely to be used by another test).

Duplicating the validation is wrong because Don't Repeat Yourself.

Someday, I want logins to feed a persistent mapping (in the abstract sense) to get rid of catalog dependency during the authentication procedure, in turn allowing to get rid of caches because code will just be fast by design. Then, the check will necessarily happen synchronously in the transaction which enabled the login (refusing to insert in the mapping when an entry already exists). Which will happen when reaching "validated" state, which means during the validate transition.

Doing otherwise will just make such improvement harder for no good reason.

So I NAK this merge request.

Moving the validation is wrong as it puts real use cases at risk for an artificial one.

I agree this put some use cases at risk, generally speaking, we can say that passing a transition without checking data integrity constraints always put some use cases at risk. Some of these uses cases could be more catastrophic from end user point of view than disabling access to a login (things like business rules are not enforced, figures in the ERP becomes wrong)... But it's our workflow design to allow workflow method transitions without checking consistency and to check consistency only user transitions.

I can imagine bad things happening after this change, but I don't see why we should make an exception to the workflow rule. This is not really different from other workflows, is it ?

Someday, I want logins to feed a persistent mapping (in the abstract sense) to get rid of catalog dependency during the authentication procedure, in turn allowing to get rid of caches because code will just be fast by design. Then, the check will necessarily happen synchronously in the transaction which enabled the login (refsing to insert in the mapping when an entry already exists). Which will happen when reaching "validated" state, which means during the validate transition.

This sounds nice, but isn't it possible to implement this as:

• verify before validate_action transition that entry is not here, in a constraint returning a message targeted at end user.
• after validate transition, insert the entry. At this stage, if it's already here, just fail loudly.

Both transitions happening in the same transition, it seems possible.

My concern is really that we should not have an exception to the rule here.

I can imagine bad things happening after this change, but I don't see why we should make an exception to the workflow rule. This is not really different from other workflows, is it ?

So the logic here is: the rule leads to broken results in at least some cases, but we must still follow it and never question its validity ?

Sorry, I refuse this logic.

[EDIT]: Not to mention that checking the same thing in 2 places clearly violates the "Do not repeat yourself" principle. The implementations will eventually diverge, leading to obviously inconsistent results, and get in the way of feature evolution and improvement. How can we accept to submit to this ?

Both transitions happening in the same transition, it seems possible.

The question we should really be asking is: why do we tolerate workflows to sometime produce errors which are not nice to the user ? I think this is a very fundamental UI flaw.

So the logic here is: the rule leads to broken results in at least some cases, but we must still follow it and never question its validity ?

Sorry, I refuse this logic

Then we should question this workflow design globally and hopefully come to a better rule. Really, this workflow is no different from others.