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

Add user activity logs to messaging endpoint #2691

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ciremusyoka
Copy link
Contributor

Changes / Features implemented

Adds message notifications for the following actions:

  1. user_created
  2. user_updated
  3. password_changed

Steps taken to verify this change does what is intended

  • Added tests

Before submitting this PR for review, please make sure you have:

  • Included tests
  • Updated documentation

Closes #

@ciremusyoka ciremusyoka force-pushed the log-user-profile-activities branch 2 times, most recently from 034742a to 5f62acb Compare September 2, 2024 12:13
@@ -228,6 +246,17 @@ def create(self, request, *args, **kwargs):
profile = response.data
user_name = profile.get("username")
cache.set(f"{USER_PROFILE_PREFIX}{user_name}", profile)

# send notification on creating new user account
user = User.objects.get(username__iexact=user_name)
Copy link
Member

Choose a reason for hiding this comment

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

Could we fetch user via the id instead?

user = User.objects.get(id=profile.get("id")

Comment on lines 252 to 258
send_message(
instance_id=user_name,
target_id=user.id,
target_type=USER,
user=user,
message_verb=USER_CREATED,
)
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this directly into the UserProfileSerializer class instead? I think the above additional query for the user will be unnecessary in such a case.

@@ -211,6 +219,16 @@ def update(self, request, *args, **kwargs):
username = kwargs.get("user")
response = super().update(request, *args, **kwargs)
cache.set(f"{USER_PROFILE_PREFIX}{username}", response.data)

# send notification on updating user profile
send_message(
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also move this to the serializer. Since we are sending the messaging for both create and update, we can override the serializer's save method and call send_message there. Example

We can differentiate create from update by checking self.instance. When doing an update self.instance is not None

@@ -303,6 +330,16 @@ def partial_update(self, request, *args, **kwargs):

profile.metadata = metadata
profile.save()

# send notification on updating user profile
send_message(
Copy link
Contributor

@kelvin-muchiri kelvin-muchiri Sep 6, 2024

Choose a reason for hiding this comment

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

partial_update will call the serializer's save method. If we refactor to send the message for update within the serializer, then we can get rid of sending the message here

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

Successfully merging this pull request may close these issues.

3 participants