data icon indicating copy to clipboard operation
data copied to clipboard

Fix system condition handling

Open georgehristov opened this issue 5 years ago • 8 comments

fixes #662

georgehristov avatar Aug 08 '20 08:08 georgehristov

Specific method in Model is not a good idea.

  • huge BC break
  • does not solve issue with cloning Model (and scope)

Feel free to make you own PR. I will not change this one as it fixes the issue quite nicely.

georgehristov avatar Aug 08 '20 10:08 georgehristov

huge BC break

wanted, in refs, we clone before and we can fix easily

does not solve issue with cloning Model (and scope)

please describe

as it fixes the issue quite nicely.

the issue is that we should never set system/default by default if addCondition is added by an user! and I think this PR does NOT solve it

mvorisek avatar Aug 08 '20 10:08 mvorisek

I will not support having a special method in Model for it. It should go along with Model::addCondition and Model::scope()->addCondition

The system indicator should be kept if scope cloned and used for another Model. But removed a when scope cloned and added to another scope

georgehristov avatar Aug 08 '20 10:08 georgehristov

please describe

does not solve issue with cloning Model (and scope)

I am not strictly againts a flag in a Condition, but only like allowSystemIfAlwaysTrue and default to false - in refs, we can then set to true and there will be no implicit/unwanted behaviour

mvorisek avatar Aug 08 '20 11:08 mvorisek

Will try to assign this already when references are being set and remove it completely from Condition

georgehristov avatar Aug 08 '20 11:08 georgehristov

It is not about references but liting the scope. So what is bothering you - the method name?

setSystem -> allowSystemIfAlwaysTrue (setSystemFieldIfDefinite)

georgehristov avatar Aug 08 '20 11:08 georgehristov

currently, we assume and change behaviour of Field implicitly

please reread the whole discussion - I would remove it completely, but if this makes sense for refs and possibly some usecases, then fine, but user must explicitly enable this feature per given condition

mvorisek avatar Aug 08 '20 13:08 mvorisek

Unrequested mutation of a model is forbidden by design. Definitive fields should be resolved at runtime - they must not be editable (Field::isEditable()) and the default field value must be implied dynamically as well.

mvorisek avatar Jul 30 '22 10:07 mvorisek

fixes https://github.com/atk4/data/issues/662

this PR does not fix #662 issue, this PR adds a concept of system scope which might be wanted, but the requirements are mainly to be able to loosen/remove "system" scope for soft-delete purposes and possibly cleanup "non-system" scope added during traversing

as not addressed by this PR, no tests, no feedback, closing in favor of #1054

mvorisek avatar Sep 01 '23 09:09 mvorisek