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

onboarding: Fixes bugs from #29421. #30070

Merged
merged 2 commits into from May 13, 2024

Conversation

prakhar1144
Copy link
Member

First commit: Third commit from #30063 somehow got dropped.
Second commit: #29421 (comment)

I'm not convinced that the topics are being translated properly; they're computed outside the with override_language block. Do we want to just call send_initial_realm_messages inside a with override_language block in the caller, rather than indenting everything? Seems timely but not a blocker since none of these new strings are actually translated yet.

Screenshots and screen captures:

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.

The query to fetch one of the greetings message based on
content wasn't taking translation into account that would
result in a bug in realms using non-english language.

This commit updates the query to fix the bug.
In 'send_initial_realm_messages', the topics names were not
being translated properly as they were computed outside the
with `override_language` block.

Now, we use 'send_initial_realm_messages' inside a with
'override_language' block in the caller. This fixes the bug.

Note: We are using 'override_language' block in the caller
and not within the function as it helps to avoid indenting
everything inside the function.
@prakhar1144 prakhar1144 marked this pull request as ready for review May 13, 2024 07:50
@timabbott timabbott merged commit 0cddc8a into zulip:main May 13, 2024
6 checks passed
@timabbott
Copy link
Sponsor Member

Merged, thanks @prakhar1144! I think I know what happened with the third commit; I noticed I'd neglected to do a git rebase --continue after merging that PR, but didn't realize what it meant. Thanks for catching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants