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: Add "users/{user_id}/status" endpoint to fetch a single user's status. #30059

Merged
merged 2 commits into from
May 23, 2024

Conversation

Vector73
Copy link
Collaborator

@Vector73 Vector73 commented May 11, 2024

This PR adds "users/{user_id}/status" endpoint to the API documentation which fetches the status of a specified user.
It completes the work started in #19498.

**Screenshots and screen captures:**

Screenshot 2024-05-15 221112
Screenshot 2024-05-15 222953
Screenshot 2024-05-15 223006

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.

Copy link
Collaborator

@laurynmm laurynmm left a comment

Choose a reason for hiding this comment

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

@Vector73 - Thanks for working on adding this endpoint and documentation!

I had a few notes on the endpoint implementation and on the API documentation. Let me know if you have any questions.

For your commit message in the second commit, the shared schema isn't an parameter so I was confused when I read it and looked at the changes. I'd more clearly explain what the shared schema is deduplicating in the yaml file...

Adds "/users/{user_id}/status" endpoint to the API documentation.

Creates a shared UserStatus schema that's used for the return value
of this new endpoint and for the user_status object returned by the
register queue endpoint.

zerver/views/presence.py Outdated Show resolved Hide resolved
zerver/lib/user_status.py Outdated Show resolved Hide resolved
zerver/lib/user_status.py Outdated Show resolved Hide resolved
api_docs/changelog.md Outdated Show resolved Hide resolved
zerver/openapi/zulip.yaml Show resolved Hide resolved
zerver/openapi/zulip.yaml Show resolved Hide resolved
@Vector73
Copy link
Collaborator Author

Vector73 commented May 15, 2024

@laurynmm I have addressed the feedback above and uploaded the screenshots.

@Vector73 Vector73 force-pushed the user-status-endpoint branch 2 times, most recently from d625feb to 5f11770 Compare May 16, 2024 10:11
Copy link
Collaborator

@laurynmm laurynmm left a comment

Choose a reason for hiding this comment

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

@Vector73 - Thanks for making those updates! I had a few more comments when reading through these changes again. Some of the issues here is that the original PR was out of date with some updates that have been made to the backend test classes.

One question I have is if we want to do anything about bot users as they would never have a status set. I haven't tested, but I assume that just returns an empty dict, which is probably fine...

zerver/lib/user_status.py Outdated Show resolved Hide resolved
zerver/tests/test_user_status.py Outdated Show resolved Hide resolved
zerver/tests/test_user_status.py Outdated Show resolved Hide resolved
api_docs/changelog.md Outdated Show resolved Hide resolved
zerver/openapi/zulip.yaml Show resolved Hide resolved
@Vector73 Vector73 force-pushed the user-status-endpoint branch 4 times, most recently from fd036d4 to d6c1d1e Compare May 16, 2024 14:00
@Vector73
Copy link
Collaborator Author

One question I have is if we want to do anything about bot users as they would never have a status set. I haven't tested, but I assume that just returns an empty dict, which is probably fine...

@laurynmm I tested that and it seems like for bots, "No user found" error is raised.

I have addressed the recent feedbacks.

Copy link
Collaborator

@laurynmm laurynmm left a comment

Choose a reason for hiding this comment

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

@Vector73 - Looks good! Had one small comment. When that's updated, feel free to add the "integration review" label.

Also, just keep in mind that this python usage example will need to be updated for your changes in #30115 (depending on which one gets merged first).

zerver/openapi/python_examples.py Outdated Show resolved Hide resolved
@Vector73
Copy link
Collaborator Author

@zulipbot add "integration review"

@zulipbot zulipbot added the integration review Added by maintainers when a PR may be ready for integration. label May 22, 2024

* [`GET /users/{user_id}/status`](/api/get-user-status): Added
a new endpoint to get a specified user's currently set
[status](/help/status-and-availability).
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This is not an API change, just an API documentation change, and so does not belong here.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Oh, it is, just the PR title is misleading :)

Renames `get_user_status_dict` function to `get_all_users_status_dict`
to distinguish it from new `get_user_status` function.
The documentation Creates a shared UserStatus schema that's used for
the return value of this new endpoint and for the existing user_status
objects returned by the register queue endpoint.

Co-authored-by: Suyash Vardhan Mathur <suyash.mathur@research.iiit.ac.in>

Fixes zulip#19079.
@timabbott timabbott enabled auto-merge (rebase) May 23, 2024 00:44
@timabbott
Copy link
Sponsor Member

Made a couple documentation tweaks, mainly to clarify the prior state for how to get these data. I also squashed the API documentation commit with the change adding the endpoint itself; it's generally an atomic change to add an API endpoint and document the new API endpoint. I also edited the PR description to fix #30059 (comment) and the commit message to fix #19079, which this new endpoint appears to resolve. Thanks @Vector73, @MSurfer20, and @laurynmm!

diff --git a/api_docs/changelog.md b/api_docs/changelog.md
index 9ee17a06ae..0955ccafe7 100644
--- a/api_docs/changelog.md
+++ b/api_docs/changelog.md
@@ -22,8 +22,8 @@ format used by the Zulip server that they are interacting with.
 
 **Feature level 262**:
 
-* [`GET /users/{user_id}/status`](/api/get-user-status): Added
-  a new endpoint to get a specified user's currently set
+* [`GET /users/{user_id}/status`](/api/get-user-status): Added a new
+  endpoint to fetch an individual user's currently set
   [status](/help/status-and-availability).
 
 **Feature level 261**
diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml
index 2f636af455..619848b04b 100644
--- a/zerver/openapi/zulip.yaml
+++ b/zerver/openapi/zulip.yaml
@@ -8425,7 +8425,9 @@ paths:
         Get the [status](/help/status-and-availability) currently set by a
         user in the organization.
 
-        **Changes**: New in Zulip 9.0 (feature level 262).
+        **Changes**: New in Zulip 9.0 (feature level 262). Previously,
+        user statuses could only be fetched via the [`POST
+        /register`](/api/register-queue) endpoint.
       parameters:
         - $ref: "#/components/parameters/UserId"
       responses:

@timabbott timabbott changed the title api: Add "users/{user_id}/status" endpoint to the API documentation. api: Add "users/{user_id}/status" endpoint to fetch a single user's status. May 23, 2024
@timabbott timabbott merged commit 62dfd93 into zulip:main May 23, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when a PR may be ready for integration. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants