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

typing: Add typing constants to the post register api response. #26477

Merged
merged 1 commit into from Aug 23, 2023

Conversation

Samadeolu7
Copy link
Collaborator

@Samadeolu7 Samadeolu7 commented Aug 11, 2023

Add typing constants to the response given by the post register api response This change also reflects in the front-end page_params

For the Backend changes
We added typing constants to the response returned by the post /register API. We added the constants to "fetch_initial_state_data( )" under the condition if want("realm"), this is because we believe you only need this constants when you are in a realm. There will be a separate pull request for the corresponding front-end changes.

We also edited the OpenApi documentation to reflect the change in the response of the API. the test to confirm the contents of the page_params was also edited to reflect this change. In the front-end change we will be getting the constants from page_params rather than having it defined on there.

For the Front-End changes
We got the typing_expiry_wait_period directly from page_params to be used in ./web/src/typing_events. For typing_started_wait_period and typing_stopped_waid period that was defined in ./web/src/typing_status we couldnt import page_params as it was defined outside of its root directory. We defined the constants in ./web/src/typing and passed it into typing_status.update which we in turn edited in ./web/src/typing_status to take them as parameters.

We also had to define the constants in the web/tests/typing_status.test because the values in page_params is only populated when an API call is made which we believe is not the case in the test.

Fixes:
#25656

Screenshots and screen captures:
Current documentation
image

Current documentation
Screenshot 2023-08-20 182729

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.

zerver/lib/events.py Outdated Show resolved Hide resolved
@Samadeolu7 Samadeolu7 force-pushed the issue-25656-typing-constants branch 3 times, most recently from efe61f9 to face2f6 Compare August 12, 2023 07:00
@Samadeolu7
Copy link
Collaborator Author

@zulipbot this is ready for review

@timabbott
Copy link
Sponsor Member

Posted a few comments; you should also make the web app actually use these values. See web/shared/src/typing_status.ts for the code needing to be refactored.

@timabbott
Copy link
Sponsor Member

Probably @laurynmm should do the next review on this one, once you've addressed all the above.

@Samadeolu7 Samadeolu7 force-pushed the issue-25656-typing-constants branch 2 times, most recently from bd2f74c to 5583949 Compare August 15, 2023 19:37
@Samadeolu7
Copy link
Collaborator Author

Posted a few comments; you should also make the web app actually use these values. See web/shared/src/typing_status.ts for the code needing to be refactored.

Yes we planned on having the front end changes on a separate Pull Request as i have mentioned in the description above. We will make the Pull Request in a few hours and have a link to it in the description.

@laurynmm
Copy link
Collaborator

@Samadeolu7 - Ping me when you've got this passing CI and I'll review the changes. Looks like there's a typo that needs to be fixed.

@Samadeolu7 Samadeolu7 force-pushed the issue-25656-typing-constants branch 3 times, most recently from 7e12faf to f6e965e Compare August 18, 2023 07:35
@Samadeolu7
Copy link
Collaborator Author

@laurynmm this is ready for review

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.

@Samadeolu7 - Thanks for working on adding these constants to the register queue response! I had a few specific comments about these changes that are below. Here are my main comments:

  • I think there are some definite improvements still to be made on the API documentation updates / text. Some of the existing comments that you've reused here aren't the best, so now would be a good time to fix those up.
  • Also, as part of these changes, we'll want to update the main description in the API documentation for setting typing status for these changes. You'll want to do something similar to what we do with the descriptions for the content and topic parameters of the send a message endpoint documentation.
  • Also, make sure you review our documentation on commit discipline. In general, there shouldn't be a commit in a series that fixes mistakes in an earlier commit. Instead, each commit should stand alone in the changes it's making. My recommendation here would be to merge these two commits into one or to fix the mistakes in the first commit so that the second commit only has changes to the frontend code.

Let me know if you have any questions about the above notes or the specific comments below. Thanks again for working on this!

api_docs/changelog.md Outdated Show resolved Hide resolved
zerver/openapi/zulip.yaml Outdated Show resolved Hide resolved
zproject/default_settings.py Show resolved Hide resolved
web/src/typing_events.js Outdated Show resolved Hide resolved
web/src/typing.js Outdated Show resolved Hide resolved
web/src/typing_events.js Outdated Show resolved Hide resolved
web/tests/typing_status.test.js Show resolved Hide resolved
@Samadeolu7 Samadeolu7 force-pushed the issue-25656-typing-constants branch 4 times, most recently from 005b64a to 31106bf Compare August 20, 2023 18:55
@Samadeolu7
Copy link
Collaborator Author

@laurynmm this is ready for review

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.

@Samadeolu7 - Thanks for making those updates! I did another review pass as documentation changes can take a few rounds of review to get right. I had a few comments below, let me know if you have any questions about them.

About the web-app frontend changes, I wondered if we should just put these new constants directly in page_params.ts, which would make the updates much more straightforward.

Also, I'll plan on updating the main description of /api/set-typing-status as a follow-up as those changes are likely to need a lot of care and multiple reviews to get right. And it would be good to combine those changes with updates to the developer documentation on this typing protocol as well.

api_docs/changelog.md Outdated Show resolved Hide resolved
web/tests/typing_status.test.js Outdated Show resolved Hide resolved
zerver/openapi/zulip.yaml Outdated Show resolved Hide resolved
zerver/openapi/zulip.yaml Outdated Show resolved Hide resolved
zerver/openapi/zulip.yaml Outdated Show resolved Hide resolved
zproject/default_settings.py Outdated Show resolved Hide resolved
zproject/default_settings.py Outdated Show resolved Hide resolved
web/src/typing.js Outdated Show resolved Hide resolved
web/src/typing.js Show resolved Hide resolved
zerver/openapi/zulip.yaml Outdated Show resolved Hide resolved
@Samadeolu7 Samadeolu7 force-pushed the issue-25656-typing-constants branch 3 times, most recently from d19f7cf to fcd4ef6 Compare August 23, 2023 09:38
Adds typing notification constants to the response given by
`POST /register`. Until now, these were hardcoded by clients
based on the documentation for implementing typing notifications
in the main endpoint description for `api/set-typing-status`.

This change also reflects updating the web-app frontend code
to use the new constants from the register response.

Co-authored-by: Samuel Kabuya <samuel.mwangikabuya@kibo.school>
Co-authored-by: Wilhelmina Asante <wilhelmina.asante@kibo.school>
@laurynmm laurynmm force-pushed the issue-25656-typing-constants branch from fcd4ef6 to d77498c Compare August 23, 2023 14:18
@laurynmm
Copy link
Collaborator

@Samadeolu7 - I pushed back to your changes with a few more revisions to the documentation updates. Mostly just some polishing on the text in the POST /register response and some formatting changes to match our general styles.

I didn't update any of the frontend implementation code or test changes. Just edited a few of the code comments for the constants.

If you could take a look over these updates and let me know if you have any questions or notice anything that seems unclear or inconsistent with the typing notifications protocol.

And no worries about the CI skipping the Debian 11 run. The issue is not specific to this PR or your changes.

@laurynmm laurynmm removed their request for review August 23, 2023 14:33
@Samadeolu7
Copy link
Collaborator Author

@laurynmm-Thank you very much. I can clearly see the difference in the formatting of the comments, is there a guide for that? Is there also a guide on how to write comments to meet the general style, if there are please send links.

You didn't leave anything for me to fix, Does this mean that the PR will get merged in?
If yes, Can you recommend a similar issue for me to work on?

@laurynmm
Copy link
Collaborator

@Samadeolu7 - We don't have any documentation/guides for code comments. It's mostly something one gets a feel for while working with and reading the codebase, and of course getting code review feedback :) As far as looking for another issue to work on, the best place to chat would likely be on chat.zulip.org. Feel free to send me a direct message there.

I'm going to mark this PR for integration review, which is the next step to getting it merged. My one outstanding note for the integration review is:

  • Since the changes to web/shared/src/typing_status.ts remove the hardcoded constants being used there and the mobile app uses that code, would that be a change that the mobile folks would need to have a fix/update in place before merging these frontend changes?

The follow-up documentation changes for api/set-typing-status and the developer subsystems typing indicators doc are up in #26554.

@laurynmm laurynmm added the integration review Added by maintainers when a PR may be ready for integration. label Aug 23, 2023
@timabbott timabbott merged commit 3ce7b77 into zulip:main Aug 23, 2023
17 checks passed
@timabbott
Copy link
Sponsor Member

Merged, thanks @Samadeolu7!

@showell FYI, this will cause a small merge conflict for your /events refactoring PR; send me a DM when that's rebased and I can try to integrate what prefix I can to avoid this happening again.

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