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

Use realm icons in organization switcher #392

Open
timabbott opened this issue Feb 26, 2017 · 13 comments · May be fixed by #4647 or #5139
Open

Use realm icons in organization switcher #392

timabbott opened this issue Feb 26, 2017 · 13 comments · May be fixed by #4647 or #5139
Labels
a-multi-org experimental UI/UX To be user-tested in experimental build help wanted
Milestone

Comments

@timabbott
Copy link
Member

We just merged into master and deployed to chat.zulip.org zulip/zulip#3740, which adds support for admin-uploaded realm icons. Realm icons are the same size as avatars, 100x100.

The URL is available in the /register request request, if we have realm in the want list, which I assume we already have since it contains useful settings data.

There are 2 fields that adds to the /register response:

  • realm_icon_url (the URL for the realm's icon, all we should need). This will always be present even if the user hasn't customized it (will fall back to gravatar), but only on the very latest server versions (effectively 1.6); we should put in some form of constant fallback I guess for older servers.
  • realm_icon_source (needed only for the admin UI to change it, since this controls whether to show the "delete and go back to gravatar button")
@timabbott timabbott added this to the M6 milestone Feb 27, 2017
@borisyankov
Copy link
Contributor

borisyankov commented May 3, 2017

@timabbott may we get the /favicon.png type of alias?

@timabbott
Copy link
Member Author

yeah I can work on that next.

@timabbott
Copy link
Member Author

Actually, thinking about this a bit more, I'm not entirely sure that there's a purpose in creating that alias. We need to cache several pieces of data about each realm:

  • The server URL
  • The realm icon URL
  • The realm icon image itself (at least if we want to avoid needing Internet access to access the switcher)
  • The email address and API key
  • If we want to enhance the login screens the way we have on web, the realm's human-readable name (e.g. "The Zulip development community" and description.

Is there really benefit to having a convenience URL for getting the icon URL alone?

It might instead make sense to add those extra fields to the payload returned by /api/v1/get_auth_backends, which is the thing that the app fetches from when a user enters a server URL. What do you think of that approach?

@timabbott
Copy link
Member Author

Chatted with Boris briefly, and I think we're agreed to go with this approach.

I've implemented it as GET /api/v1/server_settings, soon to be merged and deployed to chat.zulip.org.

An important detail is that there are 2 cases here:

  • (1) The realm is known based on the URL, e.g. on a single-realm server (chat.zulip.org) or on a subdomains server (example.zulipchat.com)
  • (2) The realm is not known based on the URL, e.g. a multi-realm server not using subdomains. We will likely eventually deprecate this use case, since it's kinda a pain (or move it to require them using e.g. zulip.example.com/realm1, zulip.example.com/realm2 style login URLs). But for now we need to support this. In this case, the realm_name field will not be set.

Here's the (provisional) format for the structure, straight from our backend tests for the new endpoint:

+        without_realm_schema_checker = check_dict_only([
+            ('authentication_methods', check_dict_only([
+                ('google', check_bool),
+                ('github', check_bool),
+                ('dev', check_bool),
+                ('password', check_bool),
+            ])),
+            ('realm_uri', check_string),
+            ('zulip_version', check_string),
+            ('msg', check_string),
+            ('result', check_string),
+        ])
+        with_realm_schema_checker = check_dict_only([
+            ('zulip_version', check_string),
+            ('realm_uri', check_string),
+            ('realm_name', check_string),
+            ('realm_description', check_string),
+            ('realm_icon', check_string),
+            ('authentication_methods', check_dict_only([
+                ('google', check_bool),
+                ('github', check_bool),
+                ('dev', check_bool),
+                ('password', check_bool),
+            ])),
+            ('msg', check_string),
+            ('result', check_string),
+        ])

@timabbott
Copy link
Member Author

(This means that clients such as the mobile apps will need to do something reasonable in the event that no name/description/icon are available). I guess the alternative approach we could take is providing some default values from the server in that case; I think it's reasonable to change to that model if desired since nothing is using the new endpoint yet.

@akashnimare @geeeeeeeeek FYI, since you may find this useful as a better replacement for checking whether a given server URL is a valid Zulip server than looking for zulip.ogg. Though of course we'll likely need to keep backwards-compatibility code for servers that don't support this for years).

@timabbott
Copy link
Member Author

timabbott commented May 3, 2017

Backend feature merged as 51260b7536d0d7fffc9c4583f04d1a0ccc473ecb; I'll deploy this to chat.zulip.org shortly.

I guess one advantage for using the old /get_auth_backends endpoint rather than a new endpoint name is that old one is guaranteed to exist even on older servers, so we'd need to write less client code. It'd be easy enough to add the new data to get_auth_backends as well for now if we wanted. Let me know what you think @borisyankov!

@borisyankov
Copy link
Contributor

Looks pretty good.
Backwards compatibility is always a bummer, good that you mentioned it.

@geeeeeeeeek
Copy link

@timabbott Will change server validation in Zulip-Electron using this api.

@geeeeeeeeek
Copy link

@timabbott Plus, does previous versions of Zulip have this endpoint?

@borisyankov
Copy link
Contributor

borisyankov commented Jun 1, 2017

I think it was introduced in 1.5
There definitely are versions that do not support this, not sure if we want to handle them (likely yes).

@timabbott
Copy link
Member Author

Introduced in 1.6, which isn't even released yet.

@geeeeeeeeek
Copy link

geeeeeeeeek commented Jun 1, 2017 via email

@timabbott
Copy link
Member Author

Yes, exactly, the /server_settings API endpoint is new.

@gnprice gnprice changed the title Use realm icons for quick mobile organization switcher Use realm icons in mobile organization switcher May 24, 2018
@gnprice gnprice changed the title Use realm icons in mobile organization switcher Use realm icons in organization switcher May 24, 2018
@chrisbobbe chrisbobbe added the experimental UI/UX To be user-tested in experimental build label Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-multi-org experimental UI/UX To be user-tested in experimental build help wanted
Projects
None yet
5 participants