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

api_docs: Fix missing content from API documentation. #30375

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Vector73
Copy link
Collaborator

This PR fixes the missing content from API documentation. There are some places in the documentation where the content present in zulip.yaml does not appear on the page. I have posted screenshots of some of the places where I found this bug. There might be other places too.

Screenshots and screen captures:

Before After
Screenshot 2024-06-10 121138 Screenshot 2024-06-10 121043
Screenshot 2024-06-10 120754 Screenshot 2024-06-10 145031
Screenshot 2024-06-10 122116 Screenshot 2024-06-10 120909
Screenshot 2024-06-10 210910 Screenshot 2024-06-10 120643
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.

@timabbott timabbott requested a review from laurynmm June 10, 2024 16:51
@timabbott timabbott added the maintainer review PR is ready for review by Zulip maintainers. label Jun 10, 2024
@timabbott
Copy link
Sponsor Member

Awesome, thanks for working on this! @laurynmm can you review this one?

@laurynmm
Copy link
Collaborator

@Vector73 - Thanks for working on this issue! From the screenshots, I can see that this isn't quite what we want for a few of these missing objects in the API docs.

I've got a script I run that I use to compare the current API docs on main with changes to how the API docs are rendered in a separate branch. Here's the diff for your changes here:

Diff from main - API docs
diff -r -B -u current_api_html/create-user-group.html new_api_html/create-user-group.html
--- current_api_html/create-user-group.html	2024-06-11 15:07:58.404784132 +0200
+++ new_api_html/create-user-group.html	2024-06-11 15:08:46.748221537 +0200
@@ -373,7 +373,15 @@
 <p><strong>Changes</strong>: Before Zulip 8.0 (feature level 198),
 the <code>can_mention_group</code> setting was named <code>can_mention_group_id</code>.</p>
 <p>New in Zulip 8.0 (feature level 191). Previously, groups
-could be mentioned if and only if they were not system groups.</p></div>
+could be mentioned if and only if they were not system groups.</p><p><strong>can_mention_group</strong> object details:</p>
+<ul>
+<li>
+<code>direct_members</code>: <span class=api-field-type>(integer)[]</span>  <p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<code>direct_subgroups</code>: <span class=api-field-type>(integer)[]</span>  <p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+</ul><p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p></div>
     <hr>
 </div>
 
diff -r -B -u current_api_html/get-events.html new_api_html/get-events.html
--- current_api_html/get-events.html	2024-06-11 15:08:05.145834509 +0200
+++ new_api_html/get-events.html	2024-06-11 15:08:52.363283126 +0200
@@ -4469,7 +4469,17 @@
 <p>New in Zulip 8.0 (feature level 191). Previously, groups
 could be mentioned if and only if they were not system groups.</p>
 <ul>
-<li>The ID of the <a href="/help/user-groups">user group</a> with this permission.</li>
+<li>
+<p><code>direct_members</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<p><code>direct_subgroups</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+<li>
+<p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p>
+</li>
 </ul>
 </li>
 </ul>
@@ -4535,7 +4545,17 @@
 <p>New in Zulip 8.0 (feature level 191). Previously, groups
 could be mentioned if and only if they were not system groups.</p>
 <ul>
-<li>The ID of the <a href="/help/user-groups">user group</a> with this permission.</li>
+<li>
+<p><code>direct_members</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<p><code>direct_subgroups</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+<li>
+<p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p>
+</li>
 </ul>
 </li>
 </ul>
@@ -5225,6 +5245,47 @@
 array.</p>
 <p>We consider this part of the Zulip API to be unstable; it is used only for
 UI elements for administering bots and is likely to change.</p>
+<ul>
+<li>
+<p>When the bot is an outgoing webhook bot.</p>
+<ul>
+<li>
+<p><code>base_url</code>: <span class="api-field-type">string</span></p>
+<p>The URL the outgoing webhook is configured to post to.</p>
+</li>
+<li>
+<p><code>token</code>: <span class="api-field-type">string</span></p>
+<p>A unique token that the third-party service can use to confirm
+that the request is indeed coming from Zulip.</p>
+</li>
+<li>
+<p><code>interface</code>: <span class="api-field-type">integer</span></p>
+<p>Integer indicating what format requests are posted in:</p>
+<ul>
+<li>1 = Zulip's native outgoing webhook format.</li>
+<li>2 = Emulate the Slack outgoing webhook format.</li>
+</ul>
+</li>
+</ul>
+</li>
+<li>
+<p>When the bot is an embedded bot.</p>
+<ul>
+<li>
+<p><code>service_name</code>: <span class="api-field-type">string</span></p>
+<p>The name of the bot.</p>
+</li>
+<li>
+<p><code>config_data</code>: <span class="api-field-type">object</span></p>
+<p>A "string: string" dictionary which describes the configuration
+for the embedded bot (usually details like API keys).</p>
+<ul>
+<li>String describing the config data.</li>
+</ul>
+</li>
+</ul>
+</li>
+</ul>
 </li>
 <li>
 <p><code>email</code>: <span class="api-field-type">string</span></p>
@@ -5336,6 +5397,47 @@
 array.</p>
 <p>We consider this part of the Zulip API to be unstable; it is used only for
 UI elements for administering bots and is likely to change.</p>
+<ul>
+<li>
+<p>When the bot is an outgoing webhook bot.</p>
+<ul>
+<li>
+<p><code>base_url</code>: <span class="api-field-type">string</span></p>
+<p>The URL the outgoing webhook is configured to post to.</p>
+</li>
+<li>
+<p><code>token</code>: <span class="api-field-type">string</span></p>
+<p>A unique token that the third-party service can use to confirm
+that the request is indeed coming from Zulip.</p>
+</li>
+<li>
+<p><code>interface</code>: <span class="api-field-type">integer</span></p>
+<p>Integer indicating what format requests are posted in:</p>
+<ul>
+<li>1 = Zulip's native outgoing webhook format.</li>
+<li>2 = Emulate the Slack outgoing webhook format.</li>
+</ul>
+</li>
+</ul>
+</li>
+<li>
+<p>When the bot is an embedded bot.</p>
+<ul>
+<li>
+<p><code>service_name</code>: <span class="api-field-type">string</span></p>
+<p>The name of the bot.</p>
+</li>
+<li>
+<p><code>config_data</code>: <span class="api-field-type">object</span></p>
+<p>A "string: string" dictionary which describes the configuration
+for the embedded bot (usually details like API keys).</p>
+<ul>
+<li>String describing the config data.</li>
+</ul>
+</li>
+</ul>
+</li>
+</ul>
 </li>
 <li>
 <p><code>is_active</code>: <span class="api-field-type">boolean</span></p>
@@ -5756,7 +5858,17 @@
 <code>realm_create_public_stream_policy</code> field used to control the
 permission to create public channels.</p>
 <ul>
-<li>The ID of the <a href="/help/user-groups">user group</a> with this permission.</li>
+<li>
+<p><code>direct_members</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<p><code>direct_subgroups</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+<li>
+<p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p>
+</li>
 </ul>
 </li>
 <li>
diff -r -B -u current_api_html/get-user-groups.html new_api_html/get-user-groups.html
--- current_api_html/get-user-groups.html	2024-06-11 15:07:58.263783116 +0200
+++ new_api_html/get-user-groups.html	2024-06-11 15:08:46.605219998 +0200
@@ -380,7 +380,17 @@
 <p>New in Zulip 8.0 (feature level 191). Previously, groups
 could be mentioned if and only if they were not system groups.</p>
 <ul>
-<li>The ID of the <a href="/help/user-groups">user group</a> with this permission.</li>
+<li>
+<p><code>direct_members</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<p><code>direct_subgroups</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+<li>
+<p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p>
+</li>
 </ul>
 </li>
 </ul>
diff -r -B -u current_api_html/register-queue.html new_api_html/register-queue.html
--- current_api_html/register-queue.html	2024-06-11 15:08:04.557829975 +0200
+++ new_api_html/register-queue.html	2024-06-11 15:08:51.789276732 +0200
@@ -1335,7 +1335,17 @@
 <p>New in Zulip 8.0 (feature level 191). Previously, groups
 could be mentioned if and only if they were not system groups.</p>
 <ul>
-<li>The ID of the <a href="/help/user-groups">user group</a> with this permission.</li>
+<li>
+<p><code>direct_members</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<p><code>direct_subgroups</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+<li>
+<p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p>
+</li>
 </ul>
 </li>
 </ul>
@@ -1391,6 +1401,47 @@
 array.</p>
 <p>We consider this part of the Zulip API to be unstable; it is used only for
 UI elements for administering bots and is likely to change.</p>
+<ul>
+<li>
+<p>When the bot is an outgoing webhook bot.</p>
+<ul>
+<li>
+<p><code>base_url</code>: <span class="api-field-type">string</span></p>
+<p>The URL the outgoing webhook is configured to post to.</p>
+</li>
+<li>
+<p><code>token</code>: <span class="api-field-type">string</span></p>
+<p>A unique token that the third-party service can use to confirm
+that the request is indeed coming from Zulip.</p>
+</li>
+<li>
+<p><code>interface</code>: <span class="api-field-type">integer</span></p>
+<p>Integer indicating what format requests are posted in:</p>
+<ul>
+<li>1 = Zulip's native outgoing webhook format.</li>
+<li>2 = Emulate the Slack outgoing webhook format.</li>
+</ul>
+</li>
+</ul>
+</li>
+<li>
+<p>When the bot is an embedded bot.</p>
+<ul>
+<li>
+<p><code>service_name</code>: <span class="api-field-type">string</span></p>
+<p>The name of the bot.</p>
+</li>
+<li>
+<p><code>config_data</code>: <span class="api-field-type">object</span></p>
+<p>A "string: string" dictionary which describes the configuration
+for the embedded bot (usually details like API keys).</p>
+<ul>
+<li>String describing the config data.</li>
+</ul>
+</li>
+</ul>
+</li>
+</ul>
 </li>
 <li>
 <p><code>email</code>: <span class="api-field-type">string</span></p>
@@ -3584,7 +3635,17 @@
 <code>realm_create_public_stream_policy</code> field used to control the
 permission to create public channels.</p>
 <ul>
-<li>The ID of the <a href="/help/user-groups">user group</a> with this permission.</li>
+<li>
+<p><code>direct_members</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<p><code>direct_subgroups</code>: <span class="api-field-type">(integer)[]</span></p>
+<p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+<li>
+<p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p>
+</li>
 </ul>
 </li>
 <li>
diff -r -B -u current_api_html/update-presence.html new_api_html/update-presence.html
--- current_api_html/update-presence.html	2024-06-11 15:07:57.613778462 +0200
+++ new_api_html/update-presence.html	2024-06-11 15:08:46.053214056 +0200
@@ -487,6 +487,70 @@
 <p><strong>Note</strong>: The old legacy format should only be used when
 interacting with old servers. It will be removed as soon as
 doing so is practical.</p>
+<ul>
+<li>
+<p><code>{client_name}</code> or <code>"aggregated"</code>: <span class="api-field-type">object</span></p>
+<p>Object containing the details of the user's
+presence.</p>
+<p><strong>Changes</strong>: Starting with Zulip 7.0 (feature level 178), this will always
+contain two keys, <code>"website"</code> and <code>"aggregated"</code>, with identical data. The
+server no longer stores which client submitted presence updates.</p>
+<p>Previously, the <code>{client_name}</code> keys for these objects were the names of the
+different clients where the user was logged in, for example <code>website</code> or
+<code>ZulipDesktop</code>.</p>
+<ul>
+<li>
+<p><code>client</code>: <span class="api-field-type">string</span></p>
+<p>The client's platform name.</p>
+<p><strong>Changes</strong>: Starting with Zulip 7.0 (feature level 178), this will
+always be <code>"website"</code> as the server no longer stores which client
+submitted presence data.</p>
+</li>
+<li>
+<p><code>status</code>: <span class="api-field-type">string</span></p>
+<p>The status of the user on this client. Will be either <code>"idle"</code>
+or <code>"active"</code>.</p>
+</li>
+<li>
+<p><code>timestamp</code>: <span class="api-field-type">integer</span></p>
+<p>The UNIX timestamp of when this client sent the user's presence
+to the server with the precision of a second.</p>
+</li>
+<li>
+<p><code>pushable</code>: <span class="api-field-type">boolean</span></p>
+<p>Whether the client is capable of showing mobile/push notifications
+to the user.</p>
+<p>Not present in objects with the <code>"aggregated"</code> key.</p>
+<p><strong>Changes</strong>: Starting with Zulip 7.0 (feature level 178), always
+<code>false</code> when present as the server no longer stores which client
+submitted presence data.</p>
+</li>
+</ul>
+</li>
+<li>
+<p><code>active_timestamp</code>: <span class="api-field-type">integer</span></p>
+<p>The UNIX timestamp of the last time a client connected
+to Zulip reported that the user was actually present
+(e.g. via focusing a browser window or interacting
+with a computer running the desktop app).</p>
+<p>Clients should display users with a current
+<code>active_timestamp</code> as fully present.</p>
+</li>
+<li>
+<p><code>idle_timestamp</code>: <span class="api-field-type">integer</span></p>
+<p>The UNIX timestamp of the last time the user had a
+client connected to Zulip, including idle clients
+where the user hasn't interacted with the system
+recently.</p>
+<p>The Zulip server has no way of distinguishing whether
+an idle web app user is at their computer, but hasn't
+interacted with the Zulip tab recently, or simply left
+their desktop computer on when they left.</p>
+<p>Thus, clients should display users with a current
+<code>idle_timestamp</code> but no current <code>active_timestamp</code> as
+potentially present.</p>
+</li>
+</ul>
 </li>
 </ul>
 </li>
diff -r -B -u current_api_html/update-user-group.html new_api_html/update-user-group.html
--- current_api_html/update-user-group.html	2024-06-11 15:07:58.547785164 +0200
+++ new_api_html/update-user-group.html	2024-06-11 15:08:46.898223152 +0200
@@ -382,11 +382,27 @@
 <ul>
 <li>
 <code>new</code>: <span class=api-field-type>object | integer</span> <span class="api-argument-required">required</span> <p>The new <a href="/api/group-setting-values">group-setting value</a> for who would
-have the permission to mention the group.</p>
+have the permission to mention the group.</p><p><strong>new</strong> object details:</p>
+<ul>
+<li>
+<code>direct_members</code>: <span class=api-field-type>(integer)[]</span>  <p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<code>direct_subgroups</code>: <span class=api-field-type>(integer)[]</span>  <p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+</ul><p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p>
 </li>
 <li>
 <code>old</code>: <span class=api-field-type>object | integer</span> <span class="api-argument-optional">optional</span> <p>The expected current <a href="/api/group-setting-values">group-setting value</a>
-for who has the permission to mention the group.</p>
+for who has the permission to mention the group.</p><p><strong>old</strong> object details:</p>
+<ul>
+<li>
+<code>direct_members</code>: <span class=api-field-type>(integer)[]</span>  <p>The list of IDs of individual users in the collection of users with this permission.</p>
+</li>
+<li>
+<code>direct_subgroups</code>: <span class=api-field-type>(integer)[]</span>  <p>The list of IDs of the groups in the collection of users with this permission.</p>
+</li>
+</ul><p>The ID of the <a href="/help/user-groups">user group</a> with this permission.</p>
 </li>
 </ul></div>
     <hr>

From that diff, it looks like there are 6 endpoints that are changed and 3 objects that are now being rendered in the docs:

  • get-user-groups
    • The rendered object description for can_mention_group makes it look like the ID of the user group is a field of the object and not an alternative to the object.
  • create-user-group
    • The rendered can_mention_group parameter description is unclear in a similar way to above in that this is either the object or the integer (both of which are described). The linked documentation page for describing the group-setting values is much clearer.
  • update-user-group
    • same issue as the create-user-group endpoint
  • get-events
    • same issue with can_mention_group as get-user-groups
    • the bot object services array looks okay as rendered, but could use some descriptive text adjustments for some inconsistencies. I'll put up a separate PR for that.
  • register-queue
    • same issue with can_mention_group as get-user-groups
    • same issue with services array as get-events
  • update-presence
    • The presences boject for this one may just need some descriptive text adjustments? From the example, it looks like the {client_name} or "aggregate" object may not be present? Does this depend on the value of slim_presence? I haven't looked at the code, so I'm not sure. Have the timestamps always been present in these objects or are they part of a API feature level change?

So the biggest issue is the can_mention_group as when it's rendered the text (both as a parameter and as a return value) is not clear in that it is either the ID integer or the object. I'd focus on making that look good in this PR, as the other two objects noted above probably need some revision of the text to fix them, and I can work on that while you're working on this.

Adding a description to the object in the GroupSettingValue schema looks like it could fix the can_mention_group return value issue:

Test of adding a description to GroupSettingValue schema

Screenshot from 2024-06-11 17-20-33

Maybe for the can_mention_group parameters, you could do something with a list template similar to the object details template:

Where the template would go

Screenshot from 2024-06-11 17-35-56

@Vector73
Copy link
Collaborator Author

Vector73 commented Jun 11, 2024

Yeah, the can_mention_group docs both as parameter and response looked really weird to me when I was working on this. So, I actually fixed it in my initial changes. I removed it from this PR because I want to discuss this issue first.

The screenshot of that change

Screenshot 2024-06-10 120733


I should have uploaded this after opening this PR. Does this look good?

Also, for response one it would be great if we could add a description to the object or we can add object: and integer: in place of the description but that might look odd?

@laurynmm
Copy link
Collaborator

Probably best to open discussion in #api documentation on CZO to get other folks' feedback. I think the screenshot is a bit better, but still not great. I'll note that the object template I added a few years ago with the {name} objedt details: isn't that great either.

It'd be nice to have something like the following ...

This parameter must be one of the following:

- The integer ID of the group ...

- An object with the following fields:

  - `members`: (integer)[]
     List of IDs of users ...

  - `direct`: (integer)[]
     List of IDs of users ...

@Vector73
Copy link
Collaborator Author

Vector73 commented Jun 12, 2024

OK, I have started a discussion - https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/can_mention_group.20missing.20docs/near/1823873.

@Vector73
Copy link
Collaborator Author

Vector73 commented Jun 20, 2024

@laurynmm I have fixed the rendering of can_mention_group as the parameter in:

update-user-group

Screenshot 2024-06-20 192052

create-user-group

Screenshot 2024-06-20 192019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR is ready for review by Zulip maintainers. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants