Simplify Generic Views with Django Signals about mrmap HOT 4 CLOSED

commented by @jansule on issue

Do I understand correctly that you want to use AsyncUpdateView on any arbitrary model? And by coupling the async_task to the model, you are able to reuse this view more easily, as the async task might differ or might not be used at all between different models?

If so, then following comes to my mind:

• Do we have a model, where we ALWAYS want to call an async_task on certain save-conditions?

If yes, then I would say that this concept should work fine.

If not, I would think about another way of coupling the task with the view. As far as I understand, we have to inherit from the AsyncUpdateView for each model anyways, in order to pass the respective model to the view.

We could then use method decorators to add the async_task where needed, like so (simplified example):

class AsyncUpdateView(UpdateView):
    # ...
    def form_valid(self, form, task=None):
        self.object.save()

        content = {
            "task": {
                "alert": Alert(msg=self.alert_msg, alert_type=AlertEnum.SUCCESS).render()
            },
        }
        
        if (task is not None):
            content["task"]["id"]: task.task_id

        # cause this is a async task which can take longer we response with 'accept' status
        return JsonResponse(status=202, data=content)
        
        
@method_decorator(async_task_func, 'form_valid')
class ServiceForm(AsyncUpdateView):
    model = Service
    # we do not have to overwrite from_valid() here

Using method_decorators on the views would have the benefit of having a softer coupling between model and async_task.

Please note that the two concepts can be used at the same time, depending on the answer for above mentioned question.

side note

I'm not too familiar on what's happening in the async_task of your example. But shouldn't the isActive property be set to true AFTER the async_task is finished?

from mrmap.

Do I understand correctly that you want to use AsyncUpdateView on any arbitrary model?

Yes

And by coupling the async_task to the model, you are able to reuse this view more easily, as the async task might differ or might not be used at all between different models?

Yes that's why i think we could use django signals here. Then it is a normal UpdateView which changes the is_active attribute of the Metadata model in this case.

Do we have a model, where we ALWAYS want to call an async_task on certain save-conditions?

No - we don't need to call an async_task always. So for that we could implement a custom django signal. Example:

class Metadata:
	__original_is_active = None
    
    def __init__(self, *args, **kwargs):
        super(Metadata, self).__init__(*args, **kwargs)
        self.__original_is_active = self.is_active
    ...
    
	@receiver(post_save, sender=Metadata)
    def is_active_changed(sender, instance, created, raw, using, update_fields, **kwargs):
    	if 'is_active' in update_fields and instance.is_active != instance.__original_is_active:
        	tasks.async_activate_deactivate_service(instance.id)
    

As far as I understand, we have to inherit from the AsyncUpdateView for each model anyways, in order to pass the respective model to the view.

IF the example above works, the AsyncUpdateView could be refactored to not call any tasks. I would like to only update the model with is_active field.

We could then use method decorators to add the async_task where needed, like so (simplified example):

Good solution as well to make it more generic

I’m not too familiar on what’s happening in the async_task of your example. But shouldn’t the isActive property be set to true AFTER the async_task is finished?

I think after refactoring with signals we should first save the new is_active value. And the activation task should get the new value from model.

Current implementation: The task get the instance of the model by id from the db if the celery schedule the task. After that all sub elements of the current instance will be traversed to change the is_active attribute to the new value.

from mrmap.

commented by @jansule :

I like your idea.

Currently, I see two different cases:

1. ALWAYS trigger a task when a certain value changes -> hard coupling via django signals on the model
2. SOMETIMES trigger a task when a certain value changes -> soft coupling via method decorators

This is something we will have to consider for each case. For your example, case 1 seems to be the better approach.

But there might be cases, where a value will be changed at different locations in the code, but a method should not always be triggered.

Let's say we have a value that can be changed by a user in the frontend, but will also be changed in some processing in the backend. We only want to call a method, if the value was directly set by the user.

So as long as we know for a certain case if we want a hard or a soft coupling, we can go with the appropriate pattern.

In any way, we do not have to implement the async task within the AsyncUpdateView, which sounds like a benefit to me.

So +1 on that

from mrmap.

I found a nice article with explanations where signals should be live: https://simpleisbetterthancomplex.com/tutorial/2016/07/28/how-to-create-django-signals.html

from mrmap.