Skip to content
This repository has been archived by the owner on Apr 16, 2024. It is now read-only.

FSMField and django model validation integration #120

Closed
khlumzeemee opened this issue Feb 25, 2016 · 10 comments
Closed

FSMField and django model validation integration #120

khlumzeemee opened this issue Feb 25, 2016 · 10 comments

Comments

@khlumzeemee
Copy link

Hi,
is it possible to create a condition for the object creation with the same syntax that exists for other transitions?

Say I have a default state = FsmField(default='new'), the only way I found to check the initial conditions is to use the clean or save methods and check that the id is null and then run some initial tests.

I would have been nice to be able to do it in the same fashion as all the other state related tests.

Is it possible to do it that way? How would you recommend to implement this using django-fsm?

@kmmbvnr
Copy link
Collaborator

kmmbvnr commented Feb 25, 2016

Can you please provide real use-case scenario for this feature?

Isn't Model.clean() valid place for this logic?

btw, you can try to mark save() as transition:

@transition(source='*', conditions=[can_save])
def save(self, *args, **kwargs):
    ...

@khlumzeemee
Copy link
Author

Hi,
the real use case scenario is an initial check that will be performed only when the status is set for the first time.

I was thinking about maybe something like

@transition(source=None, target='new')
def initial_status_check(self):

@kmmbvnr
Copy link
Collaborator

kmmbvnr commented Feb 26, 2016

Sorry, by real use case, I mean the specific business scenario. Not the abstract program sample. So, when, in real life you need to have the check on the first status set? Is it so different from situation, when you have FSMField(default='UNINITIALIZED') and explicit transition from UNINITIALIZED to the INITIALIZED state?

Typically, real scenarios could be solved in different ways, like this one. And I need to evaluate it, in order to understand is this required feature for the library or not.

@khlumzeemee
Copy link
Author

Hi,
the real life scenario is that a user tries to create a reservation and should get an error message if some initial conditions are not met (mainly if the resource is no longer available because somebody snatched it under his nose, or if he doesn't have the right to view/use this resource)

As you said before I could do it in the save or clean methods (with a "not self.id" check), I'm not saying this is a must have in django-fsm, just asking if there is a way to manage this initial status check within the django-fsm existing design (that's what I was asking with the "source=None, target='new'" example, could this work?)

Thanks to django-fsm I can do without many if/else statements in my code, I'd like to keep it that way :)

This "uninitialized"/ "initialized" solution could work but it would create a reservation first and then reject it, which is not the expected behavior, the record should not be created at all.

@ashwoods
Copy link
Contributor

-1
as far as I understand it this would require django-fsm to do some type of model initialization check. It can't check source=None as the model is initialized with the default, and not self.pk isn't really universal either (if you are using the django uuid field as recommended in the docs, your unsaved model already has a pk. The right place for this is using post_save or pre_save signals.

As this is too particular to the db layer used and can be solved easily using signals I wouldn't recommend adding this complexity to django-fsm.

@khlumzeemee
Copy link
Author

Hi @ashwoods ,
it's not a feature request, although the documentation is excellent I thought maybe this particular case has been overseen when writing it.

The project is 6 years old I'm almost sure somebody already had the same question before :)

If I understand correctly your recommendation is

def pre_save(self):
    if self.state = 'new':
       #do something

which is exactly what django-fsm was designed to avoid in the first place. So I'm a bit confused here, especially with the "-1" ...

@kmmbvnr
Copy link
Collaborator

kmmbvnr commented Feb 26, 2016

Ok, in total several thoughts that I have.

  • it's actually possible to mark _do_insert as transition. This would perform the requested check on new model instance save.
  • I afraid that having such functionality like check the reservation availability on a model level, could raise some problems in the future. Ex: fixture loading, or data migration. I think such kind of validation should exists separately. The good example is the new django 1.9 password validation design.
  • django-fsm does not allow to pass any additional arguments to conditions. So no permission check would not be possible anyway. The rationale for that is described here
  • Generally django-fsm behaves like an additional dynamic type check. In bug-free code TransitionNotAllowed newer thrown. Exactly like AttributeError. The library is not designed to plays well with django build-in model validation. But why actually so? I think is good idea to introduce field-level validation just need to implement validate method on a field
def validate_me(model, source_state, target_state):
     if model.user is None:
          raise ValidationError('You need a user assigned!')

class MyModel(models.Model):
    state = FSMField(state_validators = {
        'source_state': validate_me
    })

@khlumzeemee
Copy link
Author

Thanks @kmmbvnr , that's a lot of data to process, let me write some code and see how it works out and I'll come back to you :)

You raise some very valid points I hadn't thought about, my initial plan to bury everything in the condition methods has gone rogue..! :(

@kmmbvnr kmmbvnr changed the title Initial status check condition FSMField and django model validation integration Feb 26, 2016
@kmmbvnr kmmbvnr closed this as completed Feb 26, 2016
blueyed added a commit to blueyed/django that referenced this issue Sep 19, 2017
This patch handles ValidationErrors coming from `save_model` or
`save_related`.

This is useful for e.g. django-fsm, where a transition method might
throw this.
An alternative approach would be to hook into the form's validation
(`is_valid`), but this is not as straight forward and it seems like a
good behavior to handle ValidationErrors here, instead of letting them
bubble up and typically causing a 500 for the admin page.

Ref: viewflow/django-fsm#120
@blueyed
Copy link
Contributor

blueyed commented Sep 19, 2017

FYI: with #120 a ValidationError from the transitioning function would be handled in the admin.
(I understood this might be a use case, given the last comment)

@blueyed
Copy link
Contributor

blueyed commented Sep 19, 2017

As for admin integration I came up with gadventures/django-fsm-admin#74 - using a method on the instance.

@kmmbvnr

I think is good idea to introduce field-level validation just need to implement validate method on a field

Why have you decided to not do this after all?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants