Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add stop param to method save #43

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,24 @@ This generates the following data to be sent.
Published(destination='/topic/my_route_key_1', body='{"id": 1, "one": "Field One"}')
Published(destination='/topic/my_route_key_2', body='{"id": 1, "two": "Field Two"}')
```
## Stopping Publication

The `@publish` decorator adds a named parameter called `stop` to the `save` method, which is useful when you need to update an instance of a decorated model and do not want this update to be published on the broker.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!
I would only suggest to rename stop to send_to_queue with default value True. This would bring a little more clearance to the intent of the argument.

```python
from django.db import transaction
from django_outbox_pattern.payloads import Payload

from .models import Order

def callback(payload: Payload):
order_id = payload.body["order_id"]
status = payload.body["status"]
with transaction.atomic():
order = Order.objects.get(order_id=order_id)
order.status = status
order.save(stop=True)
payload.save()
```

## Publish/Subscribe commands

Expand Down
30 changes: 19 additions & 11 deletions django_outbox_pattern/decorators.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# pylint: disable=protected-access
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: What is the motivation to include this bypass? Not is possible to remove that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I could move it to the lines where there is access, I cannot remove it because I need to access the _meta.

import json
from typing import List
from typing import NamedTuple
Expand All @@ -17,17 +18,24 @@ class Config(NamedTuple):


def publish(configs: List[Config]):
def save(self, *args, **kwargs):
with transaction.atomic():
super(self.__class__, self).save(*args, **kwargs)
for config in configs:
_create_published(self, *config)

def decorator_publish(cls):
cls.save = save
return cls

return decorator_publish
def wrapper(cls):
class PublishModel(cls):
def save(self, *args, stop=False, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Change the stop to more clarify parameter, for example not_publish_message_to_queue. What do you think about this?
One docstring here joined with the documentation can be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing sugestion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though this suggestion would bring more readability, being sucint while naming function arguments sounds a good practice. if you really like to rename this arg, maybe you could call it send_to_queue, with default True value.
This would not break the reading of the method with a negative condition. What do you guys think on this?

with transaction.atomic():
super().save(*args, **kwargs)
if not stop:
for config in configs:
_create_published(self, *config)

class Meta(getattr(cls, "Meta", object)):
proxy = True
app_label = cls._meta.app_label
verbose_name = cls._meta.verbose_name
verbose_name_plural = cls._meta.verbose_name_plural

return PublishModel

return wrapper


def _create_published(obj, destination, fields, serializer, version):
Expand Down
20 changes: 18 additions & 2 deletions tests/unit/test_publish_decorator.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def setUp(self):
}

def create_user(self, model):
model.objects.create(username="test", email=self.email)
return model.objects.create(username="test", email=self.email)

def test_when_is_correct_destination(self):
destination = "queue"
Expand Down Expand Up @@ -125,7 +125,6 @@ def my_serializer_1(obj):
self.assertEqual("", published[1].body["last_name"])
self.assertEqual("", published[1].body["first_name"])
self.assertIsNone(published[1].body["last_login"])
self.assertAlmostEqual(date_joined.strftime("%Y-%m-%dT%H:%M:%S.%f")[:-3], published[1].body["date_joined"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: what do you think about use a freezegun ever than remove the line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my view freezegun is a fantastic lib and it would really be a good idea to use it. But I think that in this case it would not be worth it because it is a single line of information that is not so critical. But like I said, it's my vision. Would you like to add another lib as a dependency for this test?

self.assertFalse(published[1].body["is_superuser"])
self.assertEqual([], published[1].body["user_permissions"])
self.assertEqual("queue_2", published[1].destination)
Expand Down Expand Up @@ -170,3 +169,20 @@ def my_serializer_2(obj):
"username": "test",
},
)

def test_when_overriding_save_method(self):
def save(self, *args, **kwargs):
self.username = "test orverridden"
super(User, self).save(*args, **kwargs)

User.save = save
user_publish = publish([Config(destination="destination")])(User)
user = self.create_user(user_publish)
self.assertEqual(user.username, "test orverridden")

def test_when_stop_publishing(self):
user_published = publish([Config(destination="stop")])(User)
user = user_published(username="test", email=self.email)
user.save(stop=True)
published = Published.objects.first()
self.assertIsNone(published)