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

Add warning banner if moving topic or multiple messages to a pre-existing conversation. #31151 #31279

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add warning banner if moving topic or multiple messages to a pre-existing conversation. #31151 #31279

wants to merge 1 commit into from
+94 −0

Conversation

heyiming
Copy link
Collaborator

@heyiming heyiming commented Aug 6, 2024 •
edited
Loading

Only show the warning banner if submit button is enabled and the number of messages being moved are more than one.

Fixes: #31151

CZO thread

Screenshots and screen captures:
move_messages
move_topic

Self-review checklist

NOTE: just want to get some quick feedback so no unit test yet.

  • 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.

@heyiming heyiming force-pushed the issue-31151 branch 2 times, most recently from e96f452 to dbdf507 Compare August 8, 2024 01:24
@zulipbot zulipbot added size: L and removed size: M labels Aug 8, 2024
@heyiming heyiming changed the title WIP: Add warning banner if moving topic or multiple messages to a pre-existing conversation. #31151 Add warning banner if moving topic or multiple messages to a pre-existing conversation. #31151 Aug 8, 2024
@heyiming
Copy link
Collaborator Author

heyiming commented Aug 8, 2024 •
edited
Loading

Manual test done:

  1. Move topic to non-existing topic in current stream or other (banner not showing)
  2. Move topic to existing topic in current stream or other (banner showing)
  3. Move messages to non-existing topic in current stream or other (banner not showing)
  4. Move messages to existing topic in current stream or other (banner showing)
  5. Move messages to existing topic, banner only shows when more than one messages will be moved.

@timabbott
Copy link
Member

Thanks for working on this @heyiming! Can you clean up the commit history following our guidelines?

Check out our GitHub guide and commit guidelines for more details.

@heyiming heyiming force-pushed the issue-31151 branch 2 times, most recently from 0eede65 to de4f17a Compare August 16, 2024 01:14
Joelute
Joelute reviewed Aug 21, 2024
View reviewed changes
Copy link
Collaborator

@Joelute Joelute left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Just need some clean up and polishing. Would go over our commit guide and follow it to fix up your commit message. Your summary is too long; a max of 72 letters. Also remember to run a lint to fix up the lint issues :)

web/src/stream_popover.js Outdated Show resolved Hide resolved
web/src/stream_popover.js Outdated Show resolved Hide resolved
web/templates/modal_banner/message_moving_warning_banner.hbs Show resolved Hide resolved
zulip Outdated Show resolved Hide resolved
@alya
Copy link
Contributor

alya commented Aug 22, 2024

I think there's a chat.zulip.org thread related to this PR. Please link to it from the PR description.

@heyiming heyiming force-pushed the issue-31151 branch 2 times, most recently from 6c15600 to a9c4b19 Compare August 22, 2024 01:01
@alya alya added the maintainer review PR is ready for review by Zulip maintainers. label Sep 9, 2024
@alya
Copy link
Contributor

alya commented Sep 9, 2024

@evykassirer would it make sense for you to review this one? I haven't tested.

@heyiming heyiming force-pushed the issue-31151 branch 3 times, most recently from e20c73d to f2e0aa9 Compare September 14, 2024 12:21
@heyiming
Copy link
Collaborator Author

@alya This pr is ready for another round of review.

@alya
Copy link
Contributor

alya commented Sep 16, 2024

Please clean up your commit history and post again to request a review. See here for guidelines.

@Joelute
Copy link
Collaborator

Joelute commented Sep 16, 2024 •
edited
Loading

Yeah, we also don't like to merge changes like that, it makes for a messy commit history. Check out git guide to fix up the commit history and use git fetch upstream; git rebase upstream/main from development guide to keep your branch up to date.

@heyiming
Message move: Show warning on moving messages to a conversation.
55b6ecf 
Only show the warning banner if submit button is enabled and the number of
 messages being moved are more than one.

Fixes: zulip#31151
@heyiming
Copy link
Collaborator Author

@alya commit history should be clean now.

Joelute
Joelute reviewed Sep 18, 2024
View reviewed changes
web/src/stream_popover.js
Comment on lines +584 to +596
// target topic
if (stream_widget_value !== undefined) {
const {new_topic_name} = get_params_from_form();
if (
!stream_topic_history.stream_has_topic(
Number.parseInt(stream_widget_value, 10),
new_topic_name,
)
) {
$("#move_topic_modal .move_topic_form_warning_container").html("");
return;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we can simplify this nested if statement into two statements on the same level.

@Joelute
Copy link
Collaborator

Joelute commented Sep 18, 2024

I would also like to see more in the commit description. It's very brief and you've stated what you've did in the commit, but I want to also know your motivation and reasoning behind this commit so that contributors like you can later come back to this commit and have a clear context of what happened.

@zulipbot
Copy link
Member

Heads up @heyiming, 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
Reviewers

@Joelute Joelute Joelute left review comments

@timabbott timabbott timabbott left review comments

Assignees
No one assigned
Labels
has conflicts maintainer review PR is ready for review by Zulip maintainers. size: L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add warning banner if moving topic or multiple messages to a pre-existing conversation.
5 participants
@heyiming @timabbott @alya @Joelute @zulipbot