compose: Check posting policy for direct messages. #25572

brijsiyag · 2023-05-12T18:43:56Z

Prior this commit, changing the message type from a stream (where posting was not allowed) to a direct message using the compose box dropdown, did not changed the state of the submit button from disabled to enabled even though direct messages were allowed in the organization. Now, it checks if direct messages are allowed or not, and depending on that, it updates the send button's state, tooltip and displays a relevant banner.

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.

alya · 2023-05-12T19:41:18Z

Looks good to me, other than the banner text change, which should be reverted.

brijsiyag · 2023-05-12T19:46:54Z

I have updated it, @alya, can you take look at updated screen capture(2nd one)? I think tooltip looks odd in this case. Should we increase width a little bit ?

alya · 2023-05-12T20:17:46Z

Can you please post just a screenshot with the corrected banner? In general, always use a screenshot, as the PR template notes.

Screenshots are easier for me to review than videos, which can be nice additions, but aren't a replacement.

alya · 2023-05-12T20:18:05Z

Similarly, a screenshot would help review the tooltip question.

web/src/compose_recipient.js

timabbott · 2023-05-12T20:22:39Z

web/src/tippyjs.js

+            }
+            return $t({
+                defaultMessage: "You do not have permission to post in this stream.",
+            });


It would be nice to avoid the code duplication that we have for these strings. One idea would be to extract a compose_recipient.get_posting_policy_error_message function that returns "" or the error string depending on whether you're allowed to send messages, and use that both here and in check_posting_policy_for_compose_box.

Okay what I commented earlier was pending for 10 hours and now got deleted.

I was saying that one thing to notice is I have created a new function in stream_data which is get_sub_by_id, to avoid 3 calls to get can_post_messages_in_stream as "get stream name", "get stream" and "get can_post_messages_in_stream", Now it is using 2 calls.

Should I do this in a separate commit?

brijsiyag · 2023-05-12T21:57:18Z

@alya , I have updated Screenshots. I was talking about tooltip in first Screenshot, as there is too empty space at end which gives some odd look.

alya · 2023-05-12T23:10:41Z

Got it. Sure, giving the tooltips a bit more space so that the text fits in English seems helpful (in a separate commit). Thanks!

brijsiyag · 2023-05-13T07:50:50Z

Sure, So I am doing maxWidth to 350, which works on most of the zoom-ins and zoom-outs.

alya · 2023-05-15T21:01:58Z

Sure, So I am doing maxWidth to 350, which works on most of the zoom-ins and zoom-outs.

Thanks! You should post updated screenshots. :)

timabbott · 2023-05-15T22:14:46Z

This mostly lgtm but I'm going to defer merging until screenshots are posted. Thanks @brijsiyag!

web/src/compose_recipient.js

brijsiyag · 2023-05-16T00:33:59Z

Sure, So I am doing maxWidth to 350, which works on most of the zoom-ins and zoom-outs.

Thanks! You should post updated screenshots. :)

Ooh yes I should have, an another bug caught my attention and I forgot to do so.
Updated now !!

web/src/compose_recipient.js

brijsiyag · 2023-05-16T01:43:00Z

@timabbott , I have updated the code, PTAL.

alya · 2023-05-16T06:52:23Z

The updated screenshots look good to me. @timabbott let me know if you'd like me to retest this PR.

timabbott · 2023-05-16T17:21:20Z

I got rid of get_sub_by_id in favor of directly calling sub_store.get(), which seems to have the same output; otherwise lgtm, marked to merge once CI passes. Thanks @brijsiyag!

(Please comment if you see any problem with my changes).

brijsiyag · 2023-05-16T17:36:49Z

@timabbott , why I'm not able to see the changes in compose_recipient.js ?
Seems like you missed that, Should I push with updates ?

Prior this commit, changing the message type from a stream (where posting was not allowed) to a direct message using the compose box dropdown, did not changed the state of the send button from disabled to enabled even though direct messages were allowed in the organization. This was happening because `check_stream_posting_policy_for_compose_box` was only for streams. Now, function is updated to check for both streams and direct messages, as it checks if direct messages are allowed or not, and depending on that, it updates the send button's state, tooltip and displays a relevant banner.

Changes the tooltip max-width on the disabled send button from 300px to 350px to remove the weird looking padding at the end.

timabbott · 2023-05-16T17:57:14Z

Fixed.

After the changes in zulip#25572, users were no longer able to start a direct message with bots if the organization disabled direct messages. However, we should allow direct messages to bots regardless of the policy because it's a useful interface for users to interact with various classes of bots. user_ids_string_to_ids_array was also modified to prevent cases where the split function returned an array of [0] rather than [] when dealing with a empty id string of "". Fixes: zulip#21896.

After the changes in #25572, users were no longer able to start a direct message with bots if the organization disabled direct messages. However, we should allow direct messages to bots regardless of the policy because it's a useful interface for users to interact with various classes of bots. user_ids_string_to_ids_array was also modified to prevent cases where the split function returned an array of [0] rather than [] when dealing with a empty id string of "". Fixes: #21896.

After the changes in zulip#25572, users were no longer able to start a direct message with bots if the organization disabled direct messages. However, we should allow direct messages to bots regardless of the policy because it's a useful interface for users to interact with various classes of bots. user_ids_string_to_ids_array was also modified to prevent cases where the split function returned an array of [0] rather than [] when dealing with a empty id string of "". Fixes: zulip#21896.

zulipbot added the size: L label May 12, 2023

brijsiyag force-pushed the dm_send_disabled branch 3 times, most recently from 8e9d2d0 to 7516b71 Compare May 12, 2023 19:39

alya added the release goal We'd like to resolve this for the current major release label May 12, 2023

timabbott reviewed May 12, 2023

View reviewed changes

web/src/compose_recipient.js Outdated Show resolved Hide resolved

timabbott reviewed May 12, 2023

View reviewed changes

brijsiyag force-pushed the dm_send_disabled branch from 7516b71 to 66ae2af Compare May 12, 2023 21:49

brijsiyag force-pushed the dm_send_disabled branch 4 times, most recently from 0055e2b to 085e2a5 Compare May 15, 2023 17:33

timabbott reviewed May 15, 2023

View reviewed changes

web/src/compose_recipient.js Show resolved Hide resolved

brijsiyag force-pushed the dm_send_disabled branch from 085e2a5 to 4cf2fbc Compare May 16, 2023 01:12

brijsiyag commented May 16, 2023

View reviewed changes

web/src/compose_recipient.js Show resolved Hide resolved

alya added the integration review Added by maintainers when a PR may be ready for integration. label May 16, 2023

timabbott force-pushed the dm_send_disabled branch from 4cf2fbc to 7ff97d1 Compare May 16, 2023 17:20

timabbott enabled auto-merge (rebase) May 16, 2023 17:21

brijsiyag added 2 commits May 16, 2023 10:49

tooltip: Update max-width of disabled send button tooltip.

5179cd0

Changes the tooltip max-width on the disabled send button from 300px to 350px to remove the weird looking padding at the end.

timabbott force-pushed the dm_send_disabled branch from 7ff97d1 to 5179cd0 Compare May 16, 2023 17:50

timabbott merged commit 17ec4f1 into zulip:main May 16, 2023
6 checks passed

Joelute mentioned this pull request Sep 6, 2023

compose: Allow the user to direct message bots despite of policy. #26650

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Check posting policy for direct messages. #25572

compose: Check posting policy for direct messages. #25572

brijsiyag commented May 12, 2023 •

edited

alya commented May 12, 2023

brijsiyag commented May 12, 2023 •

edited

alya commented May 12, 2023

alya commented May 12, 2023

timabbott May 12, 2023

brijsiyag May 13, 2023

brijsiyag commented May 12, 2023 •

edited

alya commented May 12, 2023

brijsiyag commented May 13, 2023

alya commented May 15, 2023

timabbott commented May 15, 2023

brijsiyag commented May 16, 2023 •

edited

brijsiyag commented May 16, 2023

alya commented May 16, 2023

timabbott commented May 16, 2023

brijsiyag commented May 16, 2023 •

edited

timabbott commented May 16, 2023

compose: Check posting policy for direct messages. #25572

compose: Check posting policy for direct messages. #25572

Conversation

brijsiyag commented May 12, 2023 • edited

alya commented May 12, 2023

brijsiyag commented May 12, 2023 • edited

alya commented May 12, 2023

alya commented May 12, 2023

timabbott May 12, 2023

Choose a reason for hiding this comment

brijsiyag May 13, 2023

Choose a reason for hiding this comment

brijsiyag commented May 12, 2023 • edited

alya commented May 12, 2023

brijsiyag commented May 13, 2023

alya commented May 15, 2023

timabbott commented May 15, 2023

brijsiyag commented May 16, 2023 • edited

brijsiyag commented May 16, 2023

alya commented May 16, 2023

timabbott commented May 16, 2023

brijsiyag commented May 16, 2023 • edited

timabbott commented May 16, 2023

brijsiyag commented May 12, 2023 •

edited

brijsiyag commented May 12, 2023 •

edited

brijsiyag commented May 12, 2023 •

edited

brijsiyag commented May 16, 2023 •

edited

brijsiyag commented May 16, 2023 •

edited