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

settings: Add initial implementation for custom welcome bot message. #29452

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

akarsh-jain-790
Copy link
Collaborator

@akarsh-jain-790 akarsh-jain-790 commented Mar 25, 2024

This commit extends the functionality of the Welcome Bot to include the custom welcome message configured by administrators. It ensures that new users receive the additional custom message along with the standard welcome message upon joining the organization.

Fixes: #27663

Screenshots and screen captures:

Screenshot with Checkbox Unchecked:

Organization settings page with the "Add a custom Welcome Bot message for new users" checkbox unchecked.
Screenshot 2024-03-26 at 6 57 28 PM

Screenshot with Checkbox Checked:

Organization settings page with the "Add a custom Welcome Bot message for new users" checkbox checked.
Screenshot 2024-03-30 at 2 50 51 PM

Save button is disabled when textarea is empty.

Screenshot 2024-03-30 at 2 49 34 PM

When attempting to send a Test Custom Welcome Bot Message with an empty message:

Screenshot 2024-03-30 at 2 49 02 PM

When attempting to send a Test Custom Welcome Bot Message with a message:

Screenshot 2024-03-30 at 2 48 19 PM

Screenshot with Custom Welcome Bot Message Included:

Screenshot 2024-04-05 at 9 34 31 AM

Screenshot without Custom Welcome Bot Message:

Screenshot 2024-03-26 at 7 04 51 PM
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.

@zulipbot
Copy link
Member

Hello @zulip/server-bots, @zulip/server-onboarding members, this pull request was labeled with the "area: onboarding", "area: bots" labels, so you may want to check it out!

@akarsh-jain-790 akarsh-jain-790 force-pushed the issue--27663 branch 3 times, most recently from fca5af3 to c7d7fbb Compare March 26, 2024 13:26
@akarsh-jain-790 akarsh-jain-790 force-pushed the issue--27663 branch 2 times, most recently from a4ba94e to bbbf746 Compare March 26, 2024 20:03
@akarsh-jain-790 akarsh-jain-790 force-pushed the issue--27663 branch 5 times, most recently from 5b5992f to 41d6424 Compare March 27, 2024 13:29
@akarsh-jain-790 akarsh-jain-790 marked this pull request as ready for review March 27, 2024 13:30
@akarsh-jain-790
Copy link
Collaborator Author

The CI pipeline is currently failing due to an issue that I've reported in CZO (https://chat.zulip.org/#narrow/stream/9-issues). Once the PR #29474 is merged, it should resolve the CI failures.

@alya
Copy link
Contributor

alya commented Mar 29, 2024

Thanks! As indicated in the issue, the custom message should be a separate message.

@alya
Copy link
Contributor

alya commented Mar 29, 2024

Also, the field label doesn't match the description in the issue:

Add a "Message text" text box field

Copy link
Member

@prakhar1144 prakhar1144 left a comment

Choose a reason for hiding this comment

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

Thanks for keeping the PR updated & sorry for the delay in reviewing it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, we should better name the file as welcome_bot_custom_message and similar change at other places?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I will update the file name to welcome_bot_custom_message and make similar changes in other relevant places.

@@ -131,6 +134,35 @@ def send_initial_direct_message(user: UserProfile) -> None:
disable_external_notifications=True,
)

custom_welcome_bot_message_string = ""
custom_welcome_bot_message_text = ""
Copy link
Member

Choose a reason for hiding this comment

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

I think, _string and _text suffix don't create much distinction & are confusing to understand how they differ.

welcome_bot_custom_test_message_content (or simply welcome_bot_test_message_content) & welcome_bot_custom_message_content sounds better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll update the variable names accordingly.

user.realm.custom_welcome_bot_message_enabled
and user.realm.custom_welcome_bot_message is not None
):
custom_welcome_bot_message_text = user.realm.custom_welcome_bot_message.strip()
Copy link
Member

Choose a reason for hiding this comment

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

The text stored in database should be stripped before saving.

custom_welcome_bot_message_text = custom_welcome_bot_message.strip()
elif (
user.realm.custom_welcome_bot_message_enabled
and user.realm.custom_welcome_bot_message is not None
Copy link
Member

Choose a reason for hiding this comment

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

We should use assert user.realm.custom_welcome_bot_message is not None, right ?


if custom_welcome_bot_message_text:
custom_welcome_bot_message_string = (
_(
Copy link
Member

Choose a reason for hiding this comment

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

This won't get translated properly as it is outside with override_language(user.default_language): block.

Also, recently we have started using remove_single_newlines(), we should use that here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. However, the quote\n{realm_custom_welcome_bot_message}\n block requires the newlines to maintain the correct formatting. Using remove_single_newlines here would disrupt the intended structure of the message.


internal_send_private_message(
get_system_bot(settings.WELCOME_BOT, user.realm_id),
user,
Copy link
Member

Choose a reason for hiding this comment

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

We can use internal_prep_private_message() twice, instead of using internal_send_private_message() twice. and then use do_send_messages([default_message, custom_message]) -- This will save a lot of db queries.

zproject/urls.py Outdated
@@ -300,6 +301,8 @@
),
# realm/deactivate -> zerver.views.deactivate_realm
rest_path("realm/deactivate", POST=deactivate_realm),
# realm/test_custom_welcome_bot_message -> zerver.views.send_test_custom_welcome_bot_message
Copy link
Member

Choose a reason for hiding this comment

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

zerver.views.send_test_custom_welcome_bot_message -- no such file exists

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad, I thought it was a function as the above comment has "deactivate_realm," which is not a file name but a function inside realm.py. I'll correct it. Thanks for pointing it out.

This commit extends the functionality of the Welcome Bot to
include the custom welcome message configured by administrators. It
ensures that new users receive the additional custom message along
with the standard welcome message upon joining the organization.

Fixes: zulip#27663
@akarsh-jain-790
Copy link
Collaborator Author

@prakhar1144 I have updated the PR

@zulipbot
Copy link
Member

Heads up @akarsh-jain-790, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

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

Successfully merging this pull request may close these issues.

Make it possible to add a custom message to Welcome Bot
4 participants