bots: Only send avatar_url from backend when client cannot compute it. #25073

Ujjawal3 · 2023-04-11T12:10:06Z

Earlier realm_bots data sent in page_params contain avatar_url for all cases. This commit changes this behaviour and now avatar_url is only sent when client cannot compute it and also make changes in frontend for computing the avatar_url when not sent from backend.

Fixes: #24698

Screenshots and screen captures:

Self-review checklist

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

Communicate decisions, questions, and potential concerns.

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

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

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

Completed manual review and testing of the following:

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

Ujjawal3 · 2023-04-11T17:32:51Z

@sahil839 please review this PR.

sahil839

Posted few comments. Also, there is a typo in second commit message. It should be "deprecated".

sahil839 · 2023-04-13T14:27:11Z

zerver/openapi/zulip.yaml

@@ -416,6 +416,7 @@ paths:
                                            The ID of the user who is affected by this change.
                                        avatar_url:
                                          type: string
+                                          nullable: true


You need to add **Changes** entry here and also the appropriate changes in changelog.md and version.py.

sahil839 · 2023-04-13T14:29:12Z

zerver/actions/users.py

+                avatar_source=botdict["avatar_source"],
+                avatar_version=botdict["avatar_version"],
+                medium=False,
+                client_gravatar=True,


You cannot pass client_gravatar as True always. It depends on the client.

sahil839 · 2023-04-13T14:32:24Z

zerver/actions/users.py

@@ -516,7 +516,15 @@ def get_owned_bot_dicts(
            "default_events_register_stream": botdict["default_events_register_stream__name"],
            "default_all_public_streams": botdict["default_all_public_streams"],
            "owner_id": botdict["bot_owner_id"],
-            "avatar_url": avatar_url_from_dict(botdict),


If removing this line removes 100% coverage on zerver/lib/avatar.py, then the function should be removed in this commit itself, since we want each commit to pass the tests.

sahil839 · 2023-04-13T14:34:03Z

zerver/actions/create_user.py

@@ -343,7 +343,7 @@ def stream_name(stream: Optional[Stream]) -> Optional[str]:
        default_sending_stream=default_sending_stream_name,
        default_events_register_stream=default_events_register_stream_name,
        default_all_public_streams=user_profile.default_all_public_streams,
-        avatar_url=avatar_url(user_profile),
+        avatar_url=avatar_url(user_profile, client_gravatar=True),


We would need to set client_gravatar as False here, since we would not know whether client can compute the gravatar or not. notify_created_user has similar implementation.

@sahil839 making client_gravatar always equal to False here will cause various backend tests related to bots to fail. The reason for this is that many tests are calling the function implemented here. As this function matches the state received from server and states after applying events. As in states received from event avatar_url is always sent and in state received from server avatar_url is None states does not match and hence tests are failed.

We cannot assume that client can compute url for all clients by passing client_gravatar as True just for the sake of tests. Otherwise, the clients who cannot compute the url will also not recieve URL and would not be able to render the avatar correctly.

Instead we should assume that client cannot compute gravatar and send the url to all clients. For the tests, we would need to add code to events.py which sets the avatar_url to None if client can compute it. See the code for realm_user/add type event in lib/events.py.

Thanks! I was able to figure out how to fix the failing test cases.

Ujjawal3 · 2023-04-18T18:21:36Z

@sahil839 I have addressed all your comments. Please have a look.

sahil839

Please review the documentation changes once again. You have added Changes entry in the places where behavior is not changed.

sahil839 · 2023-04-24T04:34:29Z

zerver/openapi/zulip.yaml

                        description: |
                          URL for the user's avatar.

+                          **Changes**: In Zulip 7.0 (feature level 177)
+                          this will be `null` when client can compute the URL.
+


How does this PR changes the response of the users/me endpoint?

We are always passing client_gravatar=False to get_raw_user_data from get_profile_backend.

sahil839 · 2023-04-24T04:37:55Z

zerver/openapi/zulip.yaml

                        description: |
                          Present if `realm_user` is present in `fetch_event_types`.

                          The URL of the avatar for the current user at 100x100
                          resolution. See also `avatar_url_medium`.
+
+                          **Changes**: In Zulip 7.0 (feature level 177)
+                          this will be `null` when client can compute the URL.


This is also incorrect. The avatar_url field for the user themselves is always passed assuming client_gravatar=False.

Earlier `realm_bots` data sent in `page_params` contain `avatar_url` for all cases. This commit changes this behaviour and now `avatar_url` is only sent when client cannot compute it and also make changes in frontend for computing the `avatar_url` when not sent from backend and removes deprecated function `avatar_url_from_dict` in `zerver.lib.avatar` as it was no longer used. Fixes zulip#24698.

zulipbot · 2023-04-25T16:14:35Z

Heads up @Ujjawal3, 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.

zulipbot added the size: S label Apr 11, 2023

Ujjawal3 force-pushed the Issue-24698 branch from af4f88e to feb216a Compare April 11, 2023 12:11

zulipbot added size: M and removed size: S labels Apr 11, 2023

Ujjawal3 force-pushed the Issue-24698 branch from 47baf56 to a634dce Compare April 11, 2023 16:57

zulipbot added size: L and removed size: M labels Apr 11, 2023

Ujjawal3 force-pushed the Issue-24698 branch from a634dce to a77a5fb Compare April 11, 2023 17:00

zulipbot added size: M and removed size: L labels Apr 11, 2023

sahil839 reviewed Apr 13, 2023

View reviewed changes

Ujjawal3 force-pushed the Issue-24698 branch from a77a5fb to 700a40e Compare April 17, 2023 12:00

zulipbot added size: L and removed size: M labels Apr 17, 2023

Ujjawal3 force-pushed the Issue-24698 branch 6 times, most recently from fb741d0 to 4664c87 Compare April 18, 2023 17:11

zulipbot added the has conflicts label Apr 18, 2023

Ujjawal3 force-pushed the Issue-24698 branch from 4664c87 to 8f95d5b Compare April 19, 2023 06:05

zulipbot added size: XL size: L and removed size: L has conflicts size: XL labels Apr 19, 2023

Ujjawal3 force-pushed the Issue-24698 branch from c9edc80 to 4d20efd Compare April 19, 2023 08:34

Ujjawal3 force-pushed the Issue-24698 branch from 4d20efd to 09191c3 Compare April 19, 2023 08:36

zulipbot added the has conflicts label Apr 20, 2023

Ujjawal3 force-pushed the Issue-24698 branch from 09191c3 to 47be19a Compare April 20, 2023 05:50

zulipbot added has conflicts and removed has conflicts labels Apr 20, 2023

sahil839 reviewed Apr 24, 2023

View reviewed changes

Ujjawal3 force-pushed the Issue-24698 branch from 47be19a to 4a97def Compare April 24, 2023 07:21

zulipbot removed the has conflicts label Apr 24, 2023

zulipbot added the has conflicts label Apr 25, 2023

timabbott force-pushed the main branch 2 times, most recently from 4ec3636 to 88b200c Compare August 18, 2023 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bots: Only send avatar_url from backend when client cannot compute it. #25073

bots: Only send avatar_url from backend when client cannot compute it. #25073

Ujjawal3 commented Apr 11, 2023

Ujjawal3 commented Apr 11, 2023

sahil839 left a comment

sahil839 Apr 13, 2023

sahil839 Apr 13, 2023

sahil839 Apr 13, 2023

sahil839 Apr 13, 2023

Ujjawal3 Apr 15, 2023

sahil839 Apr 17, 2023

Ujjawal3 Apr 18, 2023

Ujjawal3 commented Apr 18, 2023

sahil839 left a comment

sahil839 Apr 24, 2023

sahil839 Apr 24, 2023

sahil839 Apr 24, 2023

zulipbot commented Apr 25, 2023

bots: Only send avatar_url from backend when client cannot compute it. #25073

Are you sure you want to change the base?

bots: Only send avatar_url from backend when client cannot compute it. #25073

Conversation

Ujjawal3 commented Apr 11, 2023

Ujjawal3 commented Apr 11, 2023

sahil839 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ujjawal3 commented Apr 18, 2023

sahil839 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Apr 25, 2023