javrasya / django-river

Django workflow library that supports on the fly changes ⛵
BSD 3-Clause "New" or "Revised" License
742 stars 105 forks source link

Question about workflow model design #32

Closed psychok7 closed 8 years ago

psychok7 commented 8 years ago

@javrasya I trying out django-river, and i noticed that if i do MyModel.objects.all() i get a AttributeError: type object 'MyModel' has no attribute 'objects', but if i do MyModel.workflow.get_queryset().all() it works.

Is this a bug or a design decision?? How does it work in the admin out of the box then, but when i try it manually i run into that error? Would i always have to use the .workflow to access my models manager?

I needed to make this work with Rest Framework and because of this issue i had to override the create method for example and explicitly say instance = ModelClass.workflow.create(**validated_data) instead of the normal instance = ModelClass.objects.create(**validated_data)

Thanks

javrasya commented 8 years ago

Sometimes, user may have their own custom manager on objects field. Overriding this field regardlessly is not good for a third party application. This is why I seperated it as different variable as workflow manager. So, this is design desicion as you say.

I don't actually remember the reason exactly. A situation poped up and I decided to change variable name. Now I remember the reason like it is as I mention earlier.

I see your case and I will think about it.

psychok7 commented 8 years ago

@javrasya how do you make it work with django admin out of the box then? They definitely have .objects in it

Anyways nowadays that sites are API driven its important to keep things compatible with Django Rest Framework out of the box as well (at least). If you have entire control of the codebase then .workflow should be fine but with third party apps and all a simple MyModel.objects.... would crash the app. Please revise that then and if you need help testing it let me know.

javrasya commented 8 years ago

I don't actually see the reason you use django-rivermanager for your regular actions like create etc. There must be some other problem about your code because objects manager should still remain in your model. Can you share you model code?

javrasya commented 8 years ago

There are two thing can be done via django-river workflow manager which is an instance of WorkflowObjectManagerwhich are get_objects_waiting_for_approval and get_object_count_waiting_for_approval. So these are not actually the things you want to use with django-restframework.

I mean, if you want to use those APIs of the workflow manager, you already need to use them explicitly.

psychok7 commented 8 years ago

@javrasya this is my model:

class QualificationRequest(models.Model):
    rnal_code = models.CharField(max_length=50, null=True, blank=True)
    name = models.CharField(max_length=200)
    phone = models.CharField(max_length=50)
    email = models.EmailField()

    status = StateField(editable=False)

    created_on = models.DateTimeField(auto_now_add=True)
    updated_on = models.DateTimeField(auto_now=True)

    def __str__(self):
        return self.name

Unless it's something related to this issue https://github.com/javrasya/django-river/issues/30

javrasya commented 8 years ago

Adding different manager into the model, makes django not to add default one which is objects. I realized that now.

Django admin works well because it detects default manager and works with it actually. Probably django gives it to admin. In django source code; models/base.py

        if not opts.managers or cls._requires_legacy_default_manager():
            if any(f.name == 'objects' for f in opts.fields):
                raise ValueError(
                    "Model %s must specify a custom Manager, because it has a "
                    "field named 'objects'." % cls.__name__
                )
            manager = Manager()
            manager.auto_created = True
            cls.add_to_class('objects', manager)

Django decides that, there is already a manager in this model and no need to add default one which is objects one. If you are struggle with this issue with djang-restfreamwork, then it is probably doesn't ask django and get default manger unlike django admin.

This may be a problem for django-river as well. Becuase, I didn't actually want to loose objects one. I will work on it and make it still remain with issue #33

Thank you for the report.

psychok7 commented 8 years ago

@javrasya aha... pure magic ;)

Yeah there should be an "easy" way for you to let .objects being accessible, maybe inherit from the same model manager as .objects and then add you django-river/workflow methods on top of it??

javrasya commented 8 years ago

@psychok7 automatic injection of workfllow manager is removed. If anyone want to use it, it should be used explicitly now. This is best way to do it I think. Please check issue #33