bots: Fix edit bot avatar settings.

[Ddharmani3]

Fixed gravatar not visible in edit bot form by making bot_avatar_url equal to bot.avatar_url || people.medium_avatar_url_for_person(bot) which solves the case when avatar_url is returned null.

Added new feature for completely removing user uploaded avatar by adding a delete button which shows on hovering on current avatar.

I added a delete button that appears when hovering over the current avatar. Then, I added an event listener for the delete button in the build_bot_edit_widget function. When clicked, it changes the current avatar image to the bot's gravatar using people.medium_gravatar_url_for_email(), and sets the value of a temporary variable bot_details.avatar_source to "G". This temporary avatar_source is only passed to the backend when the user confirms the change by clicking the dialog_submit_button in the edit bot form.

Updated format_user_row function in lib/users.py to also return avatar_source field in realm_user. Updated zulip.yaml and 4 other tests to incorporate the new field avatar_source in realm_user.

Follow-up: realm_bots: Only send avatar_url when client cannot compute it. #24698

Fixes: #24427

Screenshots and screen captures:

https://user-images.githubusercontent.com/64723994/225087419-0bdf357d-1f1b-4683-a0d8-0244ab0b793e.mp4

Self-review checklist

• [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

• [x] Explains differences from previous plans (e.g., issue description).
• [x] Highlights technical choices and bugs encountered.
• [x] Calls out remaining decisions and concerns.
• [x] Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

• [x] Each commit is a coherent idea.
• [x] Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

• [x] Visual appearance of the changes.
• [x] Responsiveness and internationalization.
• [x] Strings and tooltips.
• [x] End-to-end functionality of buttons, interactions and flows.
• [x] Corner cases, error conditions, and easily imagined bugs.

[zulipbot]

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (user)", "area: settings (admin/org)", "area: settings UI" labels, so you may want to check it out!

[Ddharmani3]

@sahil839, I have addressed both issues stated in #24427. As per your suggestions here, I tackled the issue of avatar_url being null for realm_user by calculating the gravatar for the user using people.medium_avatar_url_for_person() when bot.avatar_url is null, using the or operator: bot_avatar_url = bot.avatar_url || people.medium_avatar_url_for_person(bot).

To delete the current avatar, I used the approach shared in the PR message above. Please let me know if you like this approach or if you have any other suggestions.

Currently, I am updating the avatar_source in the frontend using bot.avatar_source = result.avatar_source, where result is the response received on success of the patch request on the endpoint bots/{bot_id}. This was necessary as the avatar_source was not updating in the frontend until the page was reloaded. I have asked for a solution to this issue on CZO. If you have any suggestions, please feel free to share them.

[Ddharmani3]

@sahil839 after receiving suggestions in CZO and reading Real-time push and events. I tried these changes:

@transaction.atomic(durable=True)
def do_change_avatar_fields(
    user_profile: UserProfile,
    avatar_source: str,
    skip_notify: bool = False,
    *,
    acting_user: Optional[UserProfile],
) -> None:
    user_profile.avatar_source = avatar_source
    user_profile.avatar_version += 1
    user_profile.save(update_fields=["avatar_source", "avatar_version"])
    event_time = timezone_now()
    RealmAuditLog.objects.create(
        realm=user_profile.realm,
        modified_user=user_profile,
        event_type=RealmAuditLog.USER_AVATAR_SOURCE_CHANGED,
        extra_data=str({"avatar_source": avatar_source}),
        event_time=event_time,
        acting_user=acting_user,
    )
    if user_profile.is_bot:
        bot_event = dict(
            type="realm_bot",
            op="update",
            bot=dict(
                user_id=user_profile.id,
                avatar_source=avatar_source,
            ),
        )
        transaction.on_commit(
            lambda: send_event(
                user_profile.realm,
                bot_event,
                bot_owner_user_ids(user_profile),
            )
        )
    event = dict(
        type="realm_user",
        op="update",
        person=dict(
            user_id=user_profile.id,
            avatar_source=avatar_source,
        ),
    )
    transaction.on_commit(
        lambda: send_event(
            user_profile.realm,
            event,
            active_user_ids(user_profile.realm_id),
        )
    )

    if not skip_notify:
        notify_avatar_url_change(user_profile)

but still, I am unable to update the avatar_source in frontend from the backend in real time. can you please help me with this?

[sahil839]

@Ddharmani3 This PR is currently hard to review. You can update the PR to have two different commits - one for just showing the avatar in edit modal and second to allow deleting the profile picture.

[sahil839]

@sahil839 after receiving suggestions in CZO and reading Real-time push and events. I tried these changes:

@transaction.atomic(durable=True)
def do_change_avatar_fields(
    user_profile: UserProfile,
    avatar_source: str,
    skip_notify: bool = False,
    *,
    acting_user: Optional[UserProfile],
) -> None:
    user_profile.avatar_source = avatar_source
    user_profile.avatar_version += 1
    user_profile.save(update_fields=["avatar_source", "avatar_version"])
    event_time = timezone_now()
    RealmAuditLog.objects.create(
        realm=user_profile.realm,
        modified_user=user_profile,
        event_type=RealmAuditLog.USER_AVATAR_SOURCE_CHANGED,
        extra_data=str({"avatar_source": avatar_source}),
        event_time=event_time,
        acting_user=acting_user,
    )
    if user_profile.is_bot:
        bot_event = dict(
            type="realm_bot",
            op="update",
            bot=dict(
                user_id=user_profile.id,
                avatar_source=avatar_source,
            ),
        )
        transaction.on_commit(
            lambda: send_event(
                user_profile.realm,
                bot_event,
                bot_owner_user_ids(user_profile),
            )
        )
    event = dict(
        type="realm_user",
        op="update",
        person=dict(
            user_id=user_profile.id,
            avatar_source=avatar_source,
        ),
    )
    transaction.on_commit(
        lambda: send_event(
            user_profile.realm,
            event,
            active_user_ids(user_profile.realm_id),
        )
    )

    if not skip_notify:
        notify_avatar_url_change(user_profile)

but still, I am unable to update the avatar_source in frontend from the backend in real time. can you please help me with this?

It is not clear what you have changed from the existing code here. It is always better to share changes in a diff format.

[Ddharmani3]

It is not clear what you have changed from the existing code here. It is always better to share changes in a diff format.

I tried to change this function in zerver/actions/user_settings.py:

[email protected](savepoint=False)
[email protected](durable=True)
def do_change_avatar_fields(
    user_profile: UserProfile,
    avatar_source: str,
    skip_notify: bool = False,
    *,
    acting_user: Optional[UserProfile],
) -> None:
    user_profile.avatar_source = avatar_source
    user_profile.avatar_version += 1
    user_profile.save(update_fields=["avatar_source", "avatar_version"])
    event_time = timezone_now()
    RealmAuditLog.objects.create(
        realm=user_profile.realm,
        modified_user=user_profile,
        event_type=RealmAuditLog.USER_AVATAR_SOURCE_CHANGED,
        extra_data=str({"avatar_source": avatar_source}),
        event_time=event_time,
        acting_user=acting_user,
    )
+    if user_profile.is_bot:
+      bot_event = dict(
+            type="realm_bot",
+           op="update",
+            bot=dict(
+                user_id=user_profile.id,
+                avatar_source=avatar_source,
+            ),
+        )
+        transaction.on_commit(
+            lambda: send_event(
+                user_profile.realm,
+                bot_event,
+                bot_owner_user_ids(user_profile),
+          )
+        )
+    event = dict(
+        type="realm_user",
+      op="update",
+        person=dict(
+            user_id=user_profile.id,
+          avatar_source=avatar_source,
+        ),
+   )
+    transaction.on_commit(
+        lambda: send_event(
+          user_profile.realm,
+           event,
+            active_user_ids(user_profile.realm_id),
+      )
+    )

    if not skip_notify:
        notify_avatar_url_change(user_profile)

[Ddharmani3]

@sahil839, I've split the PR into two commits. The first commit fixes the issue with showing the avatar in the edit modal when the avatar_url is null. This commit only has one line and should be easy to merge.

The second commit allows removing the current avatar if it's not a Gravatar. It works as expected, but there's one line where I update the avatar_source in the frontend using bot.avatar_source = result.avatar_source as it was not auto-updating until a page reload. I'm not sure if updating the realm_user's avatar_source in the frontend could cause any future bugs.

I tried making changes in the backend to update avatar_source in real-time (see code diff above), but it didn't work. Can you check if I'm going in the right direction and suggest any modifications? If I'm completely wrong, I can create an issue to solve the real-time updating problem, which can be addressed by someone who is more knowledgeable about Django backend after this PR is merged.

[sahil839]

For live-updating avatar_source, I think you can include the field in realm_bot event sent in notify_avatar_url_change.

[sahil839]

Also, while opening the issue, I thought that we had backend support to remove the avatar from bots and would not be much work, now I realise that we don't have it. So, would be nice to confirm first, whether we actually want to support deleting avatar for bots before moving forward. @alya FYI. I think we can support removing avatar for bots like we do for normal users, it is not too much amount of work.

@Ddharmani3 you can wait before working on this PR further.

[alya]

I think we can support removing avatar for bots like we do for normal users, it is not too much amount of work.

Sure, that makes sense to me.

[Ddharmani3]

@sahil839, I have addressed all of your feedback above. However, the approach that you suggested here did not work for updating avatar_source in real-time. In the updated PR, I have kept updating avatar_source in the frontend using bot.avatar_source = result.avatar_source. I believe that this approach is appropriate for testing the new feature's UI/UX aspect. I will create an issue as a follow-up to this PR for someone to work on the backend part of this. What are your thoughts on this?

[sahil839]

@sahil839, I have addressed all of your feedback above. However, the approach that you suggested here did not work for updating avatar_source in real-time. In the updated PR, I have kept updating avatar_source in the frontend using bot.avatar_source = result.avatar_source. I believe that this approach is appropriate for testing the new feature's UI/UX aspect. I will create an issue as a follow-up to this PR for someone to work on the backend part of this. What are your thoughts on this?

I think passing avatar_source to the event should work, but it may require some frontend changes as well. Should not be much work. I can try once I have some time.

[Ddharmani3]

I think passing avatar_source to the event should work, but it may require some frontend changes as well. Should not be much work. I can try once I have some time.

can you guide me a bit more in this direction? I want to try to solve this issue myself.

[sahil839]

The events sent from the server include avatar_source field. Now, you need to make sure that on receiving these events, client updates its data correctly from the events. server_events_dispatch.js is the starting point for this. Basically you should check whether the data available with client is updated after receiving the events.

[Ddharmani3]

@sahil839 I have tried to fix that issue. It is working as expected. can you please check if the approach is correct? Only adding: person_obj.avatar_source = person.avatar_source; in user_events.js worked.

[Ddharmani3]

@sahil839 updated the PR to address feedback. This PR is ready for review now.

[zulipbot]

Heads up @Ddharmani3, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.