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

message-type: Add support for "direct" as value for type parameter. #25154

Merged

Conversation

laurynmm
Copy link
Collaborator

For endpoints with a type parameter to indicate whether the message is a stream or direct message, POST /typing and POST /messages, adds support for passing "direct" as a value for direct messages.

Maintains support for "private" as a deprecated alias for direct messages.

Fixes #24960.


Notes:

  • For the Usage examples in the POST /messages documentation:
    • Comments now use 'direct messages' or 'DMs'
    • Updated the curl and javascript examples
    • Did not update the python example for "direct". I wasn't sure if we wanted to update that in tandem with an update to zulip-python-api; see question below.
  • I added the validation check for the type parameter that was in POST /typing to also be in POST /messages.
  • I did the refactor in the prep commit because any follow-up changes we do can be a bit clearer in that all instances of message_type_name now refer to these parameters passed to the view functions from clients. See git-grep below.
    • for POST /messages, we use "private" for the message_type string that's then used for the various function calls.
    • for POST /typing, we use "direct" for the default and updated message_type string.
  • Updates these two API endpoint documentation pages to use "direct messages" or "DMs" and also added links to relevant help center documentation.
git-grep of `message_type_name` after changes
$ git grep message_type_name
zerver/tests/test_message_send.py:        message_type_name = ["direct", "private"]
zerver/tests/test_message_send.py:        for message_type in message_type_name:
zerver/tests/test_message_send.py:        message_type_name = ["direct", "private"]
zerver/tests/test_message_send.py:        for message_type in message_type_name:
zerver/tests/test_typing.py:        message_type_name = ["direct", "private"]
zerver/tests/test_typing.py:        for message_type in message_type_name:
zerver/views/message_send.py:    message_type_name: str = REQ("type", str_validator=check_string_in(VALID_MESSAGE_TYPES)),
zerver/views/message_send.py:    message_type = message_type_name
zerver/views/typing.py:    message_type_name: str = REQ(
zerver/views/typing.py:    message_type = message_type_name

Questions:

  • In the Changes notes, I stated that "private" would be removed in a future release, but I wasn't sure if that's the case?
  • I added some TODO notes where message_type is set in both view functions, but these might not be something we plan on doing?
  • Again, as noted above, we'll likely want to update the zulip-python-api?

Screenshots and screen captures:

API changelog update

Screenshot from 2023-04-17 16-48-08

Send a message

Current documentation

Main endpoint description and usage examples

Screenshot from 2023-04-17 16-50-21
Screenshot from 2023-04-17 16-50-32
Screenshot from 2023-04-17 16-50-40
Screenshot from 2023-04-17 16-50-47

Parameters and response

Screenshot from 2023-04-17 16-50-57
Screenshot from 2023-04-17 16-51-06

Set typing status

Current documentation

Main endpoint description

Screenshot from 2023-04-17 16-49-12

Usage examples

Screenshot from 2023-04-17 16-49-30
Screenshot from 2023-04-17 16-49-36
Screenshot from 2023-04-17 16-49-44

Parameters and response

Screenshot from 2023-04-17 16-49-54
Screenshot from 2023-04-17 16-50-02

---
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.

@zulipbot zulipbot added size: XL area: api difficult Issues which we expect to be quite difficult priority: high release goal We'd like to resolve this for the current major release labels Apr 17, 2023
@zulipbot
Copy link
Member

Hello @zulip/server-api members, this pull request was labeled with the "area: api" label, so you may want to check it out!

@@ -173,6 +173,9 @@ class SendMessageRequest:
disable_external_notifications: bool = False


# Used to check for valid string values for message `type` parameters.
VALID_MESSAGE_TYPES = ["direct", "private", "stream"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure if there was another place in the backend code where it would be better to define this constant so that it can be shared by both zerver/views/message-send.py and zerver/views/typing.py?

Copy link
Sponsor Member

@timabbott timabbott Apr 17, 2023

Choose a reason for hiding this comment

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

I suppose Message.API_RECIPIENT_TYPES is the other place I'd consider, but that might be confusing. I think we should call these "recipient types", not "message types", because we have considered making "message type" be a phase reserved for special types of messages, like polls and certain types of special Notification Bot messages.

(And the API_ is designed to hint the fact that huddle exists as a concept only in the internal data model, not in the API)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. I moved this to Message.API_RECIPIENT_TYPES and added a comment. I used the comments for other API_ constants in models.py as a jumping off point for that comment, but it may need some revision.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! One drive-by nit.

Comment on lines 5505 to 5506
Send a stream or a private message.
Send a [stream](/help/streams-and-topics) or
a [direct](/help/direct-messages) message.
Copy link
Member

Choose a reason for hiding this comment

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

nit, and I see this is a pre-existing issue, but since we're looking at this sentence: this wording parses much more strongly for me as "send a stream, or send a direct message" than as "send a stream message, or send a direct message".

Probably better to just say "Send a stream message or a direct message".

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yeah that's a good improvement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. Updated in current version.

@timabbott
Copy link
Sponsor Member

Did not update the python example for "direct". I wasn't sure if we wanted to update that in tandem with an update to zulip-python-api; see question below.

Yeah, we should do that after doing a python-zulip-api release to support "direct" and auto-translate it to "private" for older servers.

Refactors instances of `message_type_name` and `message_type`
that are referring to API message type value ("stream" or
"private") to use `recipient_type_name` instead.

Prep commit for adding "direct" as a value for endpoints with a
`type` parameter to indicate whether the message is a stream or
direct  message.
For endpoints with a `type` parameter to indicate whether the message
is a stream or direct message, `POST /typing` and `POST /messages`,
adds support for passing "direct" as the preferred value for direct
messages, group and 1-on-1.

Maintains support for "private" as a deprecated value to indicate
direct messages.

Fixes zulip#24960.
@laurynmm laurynmm force-pushed the add-api-support-for-direct-as-type-parameter branch from 577aa1f to 2f3532b Compare April 18, 2023 17:33
@laurynmm
Copy link
Collaborator Author

Updates:

  • Addressed review feedback above.
  • Added one more prep commit for the refactor from using message_type_name or message_type to using recipient_type_name (where possible) for an instance where recipient_type was more appropriate.
  • For the Changes note in POST /typing, I didn't include the second sentence about clients needing to use the modern convention because the default value changed from "private" to "direct", so I imagine most clients won't need to do anything and can keep using the default.

Notes:

  • Follow-up when this is merged will be filing the zulip-python-api issue.

Screenshots:
Note that these are focused on areas that were changed due to review feedback.

API changelog feature level note

Screenshot from 2023-04-18 19-31-15

Send a message

Screenshot from 2023-04-18 19-32-20
Screenshot from 2023-04-18 19-32-30

Set typing status

Screenshot from 2023-04-18 19-31-46
Screenshot from 2023-04-18 19-32-05

@alya alya added the integration review Added by maintainers when a PR may be ready for integration. label Apr 18, 2023
@timabbott
Copy link
Sponsor Member

I didn't include the second sentence about clients needing to use the modern convention because the default value changed from "private" to "direct", so I imagine most clients won't need to do anything and can keep using the default.

Sounds good -- we actually haven't finished implementing typing notifications for stream messages in the web frontend yet.

@@ -210,12 +210,14 @@ def send_message_backend(
tz_guess: Optional[str] = REQ("tz_guess", default=None, documentation_pending=True),
time: Optional[float] = REQ(default=None, converter=to_float, documentation_pending=True),
) -> HttpResponse:
recipient_type_name = message_type_name
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 can be done a bit more cleanly be using the first positional argument to REQ, to not have set message_type_name at all.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Ahh, but it gets replaced in the next commit anyway, yeah this is pretty reasonable as an intermediate state before we redo the internals plumbing to support "direct" rather than "private".

@timabbott timabbott merged commit 2c043c6 into zulip:main Apr 18, 2023
6 checks passed
@timabbott
Copy link
Sponsor Member

Merged, huge thanks for doing this migration @laurynmm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api difficult Issues which we expect to be quite difficult integration review Added by maintainers when a PR may be ready for integration. priority: high release goal We'd like to resolve this for the current major release size: XL
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add API support for using "direct" as the message "type" for private messages
5 participants