zulip icon indicating copy to clipboard operation
zulip copied to clipboard

Hashchange: Change hash of #all-messages to #feed.

Open nimishmedatwal opened this issue 10 months ago • 4 comments

This pr makes hashchange of #all-messages to #feed, also redirecting users from #all-messages to #feed along with changing hash of help docs from /all-messages to /combined-feed along with it's references. CZO Discussion

Fixes: #27802.

Screenshots and screen captures:

https://github.com/zulip/zulip/assets/91622060/17683840-e004-4f63-b386-39cfe75490d9

image image

Self-review checklist
  • [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • [x] Explains differences from previous plans (e.g., issue description).
  • [x] Highlights technical choices and bugs encountered.
  • [x] Calls out remaining decisions and concerns.
  • [x] Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • [x] Each commit is a coherent idea.
  • [x] Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • [x] Visual appearance of the changes.
  • [x] Responsiveness and internationalization.
  • [x] Strings and tooltips.
  • [x] End-to-end functionality of buttons, interactions and flows.
  • [x] Corner cases, error conditions, and easily imagined bugs.

nimishmedatwal avatar Apr 17 '24 20:04 nimishmedatwal

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: web/src/hotkey.js

Function Unhandled Issue
process_hotkey Error: Caller should pass in a single row. id(src...
Event Count: 1 Affected Users: 2

Did you find this useful? React with a 👍 or 👎

sentry-io[bot] avatar Apr 17 '24 20:04 sentry-io[bot]

@zulipbot add "area: left sidebar (ui)" "area: documentation (user)"

nimishmedatwal avatar Apr 17 '24 20:04 nimishmedatwal

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

zulipbot avatar Apr 17 '24 20:04 zulipbot

@laurynmm Are you up for reviewing this one? I haven't tested.

alya avatar Apr 18 '24 21:04 alya

@nimishmedatwal - Thanks for working on this renaming!

I think it would be good to split these changes into two commits. One for the URL update in the web-app and one for the help center changes.

For the help center changes, you're going to need to add a URL redirect for the renaming of the "All messages" article markdown file. See zerver/lib/url_redirects.py and some of the pull requests that have updated that for help center changes.

For now, I'm removing the "maintainer review" label. Let me know if you have any questions and ping me again for a review when you've updated for those changes.

laurynmm avatar Apr 23 '24 13:04 laurynmm

@laurynmm I've made all the requested changes, can you give this another review?

nimishmedatwal avatar Apr 23 '24 19:04 nimishmedatwal

@nimishmedatwal You can use Zulipbot to re-add the maintainer review label once this PR passes tests.

alya avatar Apr 23 '24 21:04 alya

@zulipbot add "maintainer review"

nimishmedatwal avatar Apr 23 '24 22:04 nimishmedatwal

@laurynmm Thanks for the thorough review! I've corrected all the issues you pointed out, also should I change user_settings.web_home_view in server? For now I've changed it back to all_messages. Also I am not sure whether to change this instance of all_messages in help_relative_link.py

message_info = {
    "drafts": ["Drafts", "/#drafts", draft_instructions],
    "scheduled": ["Scheduled messages", "/#scheduled", scheduled_instructions],
    "recent": ["Recent conversations", "/#recent", recent_instructions],
    "all": ["Combined feed", "/#all_messages", all_instructions],
    "starred": ["Starred messages", "/#narrow/is/starred", starred_instructions],
    "direct": ["Direct message feed", "/#narrow/is/dm", direct_instructions],
    "inbox": ["Inbox", "/#inbox", inbox_instructions],
}

nimishmedatwal avatar Apr 25 '24 10:04 nimishmedatwal

I've corrected all the issues you pointed out, also should I change user_settings.web_home_view in server?

Yes, but please do that in its own commit, and include a migration to update the value for all existing users and realmuserdefaults. That can possibly be done in a follow-up PR.

timabbott avatar Apr 26 '24 20:04 timabbott

For that help_relative_link detail, that whole section seems like dead code; @drrosa may remember the context (0415e66969840aa450e9c06407921547bd4c3b93 removed the only use of it).

$ git grep 'relative\|message\|' help
help/direct-messages.md:{relative|message|direct}

timabbott avatar Apr 26 '24 21:04 timabbott

I went ahead and did the finishing work on this to make sure I was happy with the strings in the comments; with those changes, I think this is good to merge, thanks @nimishmedatwal and @laurynmm! @nimishmedatwal please take more time in your PRs to be careful -- read the surrounding code, run git log on the files you're changing, and git log -S on identifiers you don't understand -- and double-check your English writing -- it can really help reduce errors. I also ended up rewriting some of your commit messages.

For follow-ups, we still need an API change and database migration to close this out -- check the history for the "recent topics" rename to see how to do that. And then we also will want to rename variables -- e.g., git grep all_messages_view finds a function to rename.

timabbott avatar Apr 26 '24 21:04 timabbott

@timabbott sure! I will keep that in mind in future.

nimishmedatwal avatar Apr 27 '24 11:04 nimishmedatwal

For follow-ups, we still need an API change and database migration to close this out -- check the history for the "recent topics" rename to see how to do that.

Just a note here that we haven't yet done the API and database migration change for the "recent topics" rename yet, see issue #25781.

laurynmm avatar Apr 29 '24 16:04 laurynmm