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

Add support for old format for narrow links #1424

Merged
merged 3 commits into from
Sep 3, 2023

Conversation

mounilKshah
Copy link
Collaborator

@mounilKshah mounilKshah commented Aug 19, 2023

What does this PR do, and why?

This PR contains commit which extends support to the old format for narrow links
which existed before server version 2.1.0

#zulip-terminal > Support old narrow links format #T1422 #T1424
#api-documentation > Narrow URL formats

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 size: M [Automatic label added by zulipbot] area: message rendering area: UI General user interface update version parity: <=2.1 labels Aug 19, 2023
@mounilKshah mounilKshah added PR needs review PR requires feedback to proceed and removed area: UI General user interface update labels Aug 19, 2023
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.

@mounilKshah Thanks for taking this up 👍

This seems to work just fine, and the tests back this up.

I was going to comment that the test cases you added were a little confusing in places since the message and stream ids are identical, but I note we do that in the other situations too. On that basis, it may be useful to do some cleanup on this test, perhaps as prep commits if you're comfortable with that, namely refactor commits for:

  • inlining ids
  • adjusting the expected message id to be different to the stream id
  • moving the SERVER_URL to be used in the test body instead?

Re the commit text, note that notes re Fixes and the tests normally go on their own lines. I think ZT or Zulip document the former; the latter is likely assumed by Zulip in general, but can be useful to confirm and distinguish between new behavior coverage, adjustments/refactors, test cases vs tests, etc.

For followup:

  • We've discussed in the stream that we could document test styles/guidelines. If you'd like to draw together some elements into a document, that'd be useful to direct users towards. Another approach which I briefly explored was enabling pytest checks in ruff.
  • We may be able to either document or also add checks for Fixes and tests lines to the gitlint rules we use somehow, which would lower the need to give feedback on this as frequently.

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Aug 24, 2023
@neiljp neiljp added this to the Next Release milestone Aug 24, 2023
@mounilKshah mounilKshah force-pushed the support-old-narrow-link-format-1422 branch from 6065476 to cdbd2e9 Compare August 26, 2023 19:17
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Aug 26, 2023
This commit shifts test IDs for test__parse_narrow_link() to be inline
with the test cases, and extracts SERVER_URL into the test function
body.
This commit provides support for narrow links in message content
containing 'subject' instead of 'topic', which may be present in
messages before server version 2.1.0.

Test cases added.

Fixes zulip#1422.
@neiljp neiljp force-pushed the support-old-narrow-link-format-1422 branch from cdbd2e9 to 9d4e7aa Compare August 29, 2023 23:13
Minor change to reduce confusion between values of different ids in test
cases and ensure they are kept distinct for testing purposes.
@neiljp
Copy link
Collaborator

neiljp commented Sep 3, 2023

@mounilKshah Thanks for the refactor 👍 Adjusted very slightly and added in a small followup commit for message vs stream id distinction I referred to.

@neiljp neiljp merged commit 691a248 into zulip:main Sep 3, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: message rendering PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback size: XL [Automatic label added by zulipbot] version parity: <=2.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support old format for narrow links (subject in addition to topic)
3 participants