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.

Sign up for GitHub

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

Jump to bottom

Added the read_by_sender #1459

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Added the read_by_sender #1459

merged 1 commit into from
Apr 26, 2024

Conversation

sreecharan7
Copy link
Contributor

@sreecharan7 sreecharan7 commented Jan 12, 2024 •
edited by neiljp
Loading

What does this PR do, and why?

Introduction of the read_for_sender flag.

External discussion & connections

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

@zulipbot zulipbot added the size: S [Automatic label added by zulipbot] label Jan 12, 2024
neiljp
neiljp reviewed Jan 15, 2024
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@sreecharan7 Thanks for giving a 2nd try at this :)
I hope you're aware that you don't need to open a new PR to push an update?

The code changes look reasonable to me, though please pay attention to the new comment content.

Your commit text is slightly improved from the other PR, but doesn't relate to the files changed?

I often make small changes when merging, and these are small changes to be made, but this is a very small PR, and I flagged the commit issue previously.

zulipterminal/api_types.py Outdated
@@ -83,13 +83,15 @@ class PrivateComposition(TypedDict):
type: DirectMessageString
content: str
to: List[int] # User ids
read_by_sender: bool # New in ZFL 58, Zulip 4.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

These new comments are incorrect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify, the use of a comment here is fine, and in accordance with the rest of the file. However, this field was not introduced at this zulip version / feature level.

@neiljp neiljp added area: API spec version parity: 8 PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jan 15, 2024
@sreecharan7 sreecharan7 requested a review from neiljp January 16, 2024 15:41
neiljp
neiljp reviewed Jan 16, 2024
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@sreecharan7 This is fine to keep as one commit.

Including api_types in the commit title is good in the second commit you added, but note this will affect multiple files in combination.

I'd also suggest installing gitlint to help with commit message formatting.

zulipbot is also providing useful feedback here :)

I note that the zulip-terminal or general zulip notes don't appear to refer to refactoring, so I'd suggest reading up on that until we do. In this PR, while the change is internal, we are changing how we interact with the server, so I'd consider refactor to be inaccurate in this particular case, so that can be dropped.

zulipterminal/api_types.py Outdated
@@ -83,13 +83,15 @@ class PrivateComposition(TypedDict):
type: DirectMessageString
content: str
to: List[int] # User ids
read_by_sender: bool # New in ZFL 58, Zulip 4.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify, the use of a comment here is fine, and in accordance with the rest of the file. However, this field was not introduced at this zulip version / feature level.

@sreecharan7 @neiljp
api_types/model/boxes: Add read_by_sender when sending messages.
651da09 
This was added in ZFL 236 (Zulip 8.0).

Tests updated.

Fixes zulip#1456.
@neiljp neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 26, 2024
@neiljp neiljp added this to the Next Release milestone Apr 26, 2024
@neiljp neiljp merged commit 81b1e72 into zulip:main Apr 26, 2024
21 checks passed
@neiljp
Copy link
Collaborator

neiljp commented Apr 26, 2024

@sreecharan7 Thanks for working on this - I've made the final adjustments and this is now merged 🎉

@sreecharan7 sreecharan7 deleted the apiFix branch June 2, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

Assignees
No one assigned
Labels
area: API spec PR ready to be merged PR has been reviewed & is ready to be merged size: S [Automatic label added by zulipbot] version parity: 8
Projects
None yet
Milestone
Next Release
Development

Successfully merging this pull request may close these issues.

Pass new read_by_sender flag when sending messages
3 participants
@sreecharan7 @neiljp @zulipbot