boxes: Add support for renaming (no topic) messages. #946

zee-bit · 2021-03-12T12:41:50Z

This PR adds support for renaming messages that had no topic, by anyone as long as realm_allow_message_editing is True for the organization. This is the behavior that is followed by webapp for message editing events.

Fixes #837.

Ezio-Sarthak

Thanks for taking this up 👍 This provisionally works well.
There are some aspects that I'd suggest worth taking a look at, other than minor code changes in my comments:

One can't edit a topic multiple times (in a session) for messages sent from the same author?
[unrelated] In web app, changing topic name (other user's message) shows an edit marker for all following message sent by that user, which seems not to be the case if seen in ZT? (to clarify: editing in web app, while running a ZT session)

zulipterminal/ui_tools/boxes.py

tests/ui/test_ui_tools.py

zee-bit · 2021-03-29T21:16:47Z

@Ezio-Sarthak Thanks for the review. ☺️

I don't think there's any such restrictions, nor was I able to find anything similar in the organization setting. I manually tried editing a message topic almost 12 times in a session, and was able to. :)
I think I see the opposite. If I edit other user's message topic in the webapp, then I see edit markers in all the following messages in ZT, but not in webapp.

Re this PR, there's also a similar community topic editing, here: #793.
(I should probably query @kaustubh-nair about the status of that PR, and if he's busy, maybe I can take that forward. :) )

neiljp

@zee-bit I just merged a quick bugfix for #1034 (#1035) today, and wanted to follow-up on this PR since it'll need a small rebase. Most of the behavior of this I'm interested in seeing checked in the tests rather than manually - since it depends on server settings.

#793 does seem to be a good follow-up to rebase/amend, and in terms of testing I realize that the settings we have won't be updated from server->ZT, so another good follow-up would be to add event handling for those changes - otherwise to test this manually we have to change the setting and restart ZT each time?

tests/ui/test_ui_tools.py

zulipterminal/ui_tools/boxes.py

neiljp · 2021-05-16T00:29:15Z

tests/ui/test_ui_tools.py

+        if message_fixture['type'] == 'private':
+            to_vary_in_each_message['subject'] = ''


Is this what we expect in PMs, or is the key absent?

The API docs do say that we should expect an empty string here. Quote:

subject: string The topic of the message. Currently always "" for private messages, though this could change if Zulip adds support for topics in private message conversations.

I also did a manual test just to be extra sure. :)

zee-bit · 2021-05-22T13:02:14Z

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

zee-bit · 2021-07-04T14:33:37Z

zulipterminal/ui_tools/boxes.py

+                if self.message["type"] == "stream":
+                    self.model.controller.report_error(
+                        " You can't edit messages sent by other users"
+                        " with non-empty topic."
+                    )


This should be the accurate definition of "(no topic)" messages i.e. empty topic. But, I am a bit hesitant to print this in the footer (the user might not know how we represent empty topics?).
I also had You can't edit messages sent by other users which do not have "(no topic)".
Is there a more better way to report this?

After a short discussion with @prah23 via PM's we've decided to go with "You can't edit messages sent by other users that already have a topic.", because if we err towards being more specific then we might just confuse the user and bloat the footer with a very long message.

neiljp

@zee-bit The tests have more information, but putting the case ids inline will make it easier to determine that the right output is happening 👍

For the code itself, as we get closer to including all the range of edit possibilities, and given your "do we need this" in the code, I'm a little concerned it's tricky to follow the branches. We could put together a document which translates the "who can edit what" table in the zulip help pages into a clear rejection (can't edit because x) flow and mapping of those to text, given the range of inputs - which is sort of what the tests will illustrate, and be clearer with those inline ids :)

It might be useful to think about where community topic editing fits into the flow - I think we're sort of enabling this by default when we edit from our own messages but rename more, and used to seeing it work on czo.

neiljp · 2021-07-05T16:11:58Z

tests/ui/test_ui_tools.py

+            "msg_sent_by_other_with_no_topic",
+            "editing_not_allowed_for_no_topic",
+            "no_msg_body_edit_limit_with_no_topic",
+            "all_conditions_met",


Why this reordering? It complicates the diff. I could understand maybe improving the id now that it's more complex though - "all conditions" doesn't seem comparable to the other descriptions.

tests/ui/test_ui_tools.py

zee-bit · 2021-07-06T12:38:55Z

Thanks for the review Neil,
Re the "do we need this" in code, I'm 98% sure that we don't need that condition, since we should have already handled that at the top? To make the conditional branches easier to comprehend, I've put up small comments at the top that should provide much more clarity to our handling of message-editing, for now.

neiljp

@zee-bit The inlining of ids definitely helps - but there are a few other changes which may help simplify the tests that I've noted too. Reading the code again, it seems easier to follow now - particularly if one of the conditionals are moved around a little?

I'm also wondering if now or later we may want to reorder checks in order to give better error messages - but that's different than getting the new logic working :)

neiljp · 2021-07-06T15:23:43Z

tests/ui/test_ui_tools.py

+                {"sender_id": 1, "timestamp": 1},
+                True,
+                False,
+                60,
+                True,
+                " Only topic editing allowed."
+                " Time Limit for editing the message body has been exceeded.",
+                " Time Limit for editing the message has been exceeded.",
+                id="topic_edit_only_after_time_limit",


This isn't related to this feature, but the test (at first glance!) says editing should succeed here, yet in manual testing (and per the warnings/errors you added) editing (partially) succeeds with streams and fails with PMs!

Of course, this is related to the private-message conditional in the test. Now that you testing stream/private-dependent pairs, what do you think about expanding those pairs in an earlier commit to include the 'success/failure' before building on that to include the strings? That would perhaps simplify the conditionals you add too?

BTW I think it would be fine to have the inlining of the ids in a separate commit too, as I did in the black prep work.

This isn't related to this feature, but the test (at first glance!) says editing should succeed here, yet in manual testing (and per the warnings/errors you added) editing (partially) succeeds with streams and fails with PMs!

This seems to be also true for msg_body_edit_enabled?
Anyways, I've expanded both to contain values for both types of messages (stream/private).

tests/ui/test_ui_tools.py

zulipterminal/ui_tools/boxes.py

neiljp · 2021-07-06T16:09:21Z

zulipterminal/ui_tools/boxes.py

                        )
                        msg_body_edit_enabled = False
+                    # FIXME: Is this else necessary, we already handle this at the top?


I think this will be easier to reason about if you move the first conditional around as I indicate above.

zulipterminal/ui_tools/boxes.py

neiljp · 2021-07-06T16:17:23Z

zulipterminal/ui_tools/boxes.py

+                    # The remaining case is of a private message not belonging to user.
+                    self.model.controller.report_error(
+                        " You can't edit private messages sent by other users."


You're not testing this - is it already covered?

Actually, because of the equivalent conditional at the top, this case was already handled by that and returned so we never reach here.
~~Removing the top block as I suggested in one of the above comments should make this feasible.~~

I have amended the equivalent error message at the top to print the same thing as here.

If this block is not actually used, it is better to indicate it with a comment as to the condition and in this case where else it should be covered. Then you can also include an assertion or probably better raise a RuntimeError since it indicates an error in the application!

neiljp

@zee-bit The test refactoring are good clean changes making the feature commit much easier to follow 👍

This is complicated logic, so the tests are understandably complicated - but the more cleanly we can express the behavior the better, and this looking great!

tests/ui/test_ui_tools.py

neiljp · 2021-07-29T01:16:06Z

tests/ui/test_ui_tools.py

+        if message_type == "stream" and expected_footer_text_stream:
+            if expected_editing_to_succeed[message_type]:
+                report_warning.assert_called_once_with(expected_footer_text_stream)
+            else:
+                report_error.assert_called_once_with(expected_footer_text_stream)
+        elif message_type == "private" and expected_footer_text_private:
+            if expected_editing_to_succeed[message_type]:
+                report_warning.assert_called_once_with(expected_footer_text_private)
+            else:
+                report_error.assert_called_once_with(expected_footer_text_private)


There's quite a bit of repetition here, and it's not clear if the missing else clause is meaningful. Particularly with the dict approach I suggest, can you select the "report function" and then assert on it based on the type?

For each dict there could be different report_* functions for different message types. e.g. A stream message might allow editing just the message topic (so report_warning) whereas under the same conditions a private message may not allow editing at all (report_error). I think that will make the test even more complex if I understand your statement correctly?

This seems to be resolved with the change to a dict for the footer_text?

neiljp · 2021-07-29T01:28:14Z

zulipterminal/ui_tools/boxes.py

+                    # The remaining case is of a private message not belonging to user.
+                    self.model.controller.report_error(
+                        " You can't edit private messages sent by other users."


If this block is not actually used, it is better to indicate it with a comment as to the condition and in this case where else it should be covered. Then you can also include an assertion or probably better raise a RuntimeError since it indicates an error in the application!

tests/ui/test_ui_tools.py

zulipterminal/ui_tools/boxes.py

tests/ui/test_ui_tools.py

neiljp · 2021-08-12T03:08:17Z

tests/ui/test_ui_tools.py

@@ -2337,40 +2337,40 @@ def test_keypress_STREAM_MESSAGE(
                {"sender_id": 2, "timestamp": 45},
                True,
                60,
-                True,
-                False,
+                {"stream": False, "private": False},


This has changed from True->False. This exposes the logic of the test, and suggests we might consider a different value for this case, such as None?

neiljp · 2021-08-12T05:33:26Z

tests/ui/test_ui_tools.py

+        if message_type == "stream" and expected_footer_text_stream:
+            if expected_editing_to_succeed[message_type]:
+                report_warning.assert_called_once_with(expected_footer_text_stream)
+            else:
+                report_error.assert_called_once_with(expected_footer_text_stream)
+        elif message_type == "private" and expected_footer_text_private:
+            if expected_editing_to_succeed[message_type]:
+                report_warning.assert_called_once_with(expected_footer_text_private)
+            else:
+                report_error.assert_called_once_with(expected_footer_text_private)


This seems to be resolved with the change to a dict for the footer_text?

neiljp · 2021-08-12T05:54:16Z

@zee-bit While I made some comments, I'm just giving this a final look and may merge soon as-is.

This makes it clearer which test case has which id, compared to having separate lists of test case values and ids, which could get difficult to compare, especially when the lists get long. This will potentially be helpful when we add more tests for handling all range of message editing possibilities.

This renames the "msg_body_edit_enabled" parameter of EDIT_MESSAGE keypress to an expected type, since its not an input. Also, moves it to the last of the parameters where we generally put expect_*, to maintain consistency. We also move the expect_editing_to_succeed parameter to the end to maintain consistency.

This commit expands the stream/private dependent pairs, "expect_msg_body_edit_enabled" and "expect_editing_to_succeed", in EDIT_MESSAGE keypress to include success/failure for both types of message. We define both the parameters as Dictionaries with keys for stream and private, each mapping to the expected value of the parameter for that message type. This allows us to remove the explicit setting of these parameters from the test body and instead define them in the parametrize.

This commit adds tests for footer notifications to make them clearer. This allows it to be easily noticed and expanded if the footer texts are changed in a particular commit. We pass extra parameters to the test function, that holds the expected footer texts in case of stream or private message editing, and assert it with the value that the report_* functions have been called with.

This commit adds support for editing messages with no topic, depending on whether the server setting "realm_allow_message_editing" is enabled or not. Any user can now edit the topic of a topicless message (shown as "(no topic)") in ZT. This should bring message-editing functionality in ZT more closer to the behaviour of other zulip clients and the webapp in general. Tests amended. Fixes zulip#837.

neiljp · 2021-08-12T06:23:54Z

@zee-bit Thanks for getting this done! This has nicely tidied up these tests too 👍 I just pushed here with some very minor commit text changes and will merge shortly 🎉

zulipbot added size: L [Automatic label added by zulipbot] area: message editing labels Mar 12, 2021

zee-bit force-pushed the renaming-no-topic branch from d5e6229 to f49380b Compare March 12, 2021 18:57

neiljp added the PR needs review PR requires feedback to proceed label Mar 14, 2021

Ezio-Sarthak suggested changes Mar 29, 2021

View reviewed changes

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

tests/ui/test_ui_tools.py Outdated Show resolved Hide resolved

zee-bit force-pushed the renaming-no-topic branch from f49380b to 39b0e1f Compare March 29, 2021 21:01

zee-bit force-pushed the renaming-no-topic branch from 39b0e1f to 9140d88 Compare April 28, 2021 21:10

neiljp reviewed May 16, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels May 16, 2021

zee-bit force-pushed the renaming-no-topic branch 2 times, most recently from c88a1ab to 3a0b460 Compare May 21, 2021 23:08

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels May 21, 2021

zee-bit force-pushed the renaming-no-topic branch from 3a0b460 to da2f203 Compare May 22, 2021 12:53

zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels May 22, 2021

zee-bit force-pushed the renaming-no-topic branch from da2f203 to be4ff9b Compare June 2, 2021 21:46

zee-bit force-pushed the renaming-no-topic branch from be4ff9b to 75a8bb7 Compare July 4, 2021 14:26

zee-bit commented Jul 4, 2021

View reviewed changes

zee-bit force-pushed the renaming-no-topic branch 2 times, most recently from f4db7f5 to 97a3d62 Compare July 4, 2021 14:52

neiljp reviewed Jul 6, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jul 6, 2021

zee-bit force-pushed the renaming-no-topic branch 2 times, most recently from f2575ac to 27e8e3f Compare July 6, 2021 12:31

zee-bit removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jul 6, 2021

neiljp reviewed Jul 6, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jul 6, 2021

zee-bit force-pushed the renaming-no-topic branch from 27e8e3f to 9a4d781 Compare July 7, 2021 09:06

zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 7, 2021

zee-bit force-pushed the renaming-no-topic branch 4 times, most recently from 43fc74d to df4dbff Compare July 7, 2021 23:17

neiljp reviewed Jul 29, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jul 29, 2021

zee-bit force-pushed the renaming-no-topic branch from df4dbff to 7057718 Compare August 1, 2021 22:44

zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Aug 1, 2021

neiljp reviewed Aug 12, 2021

View reviewed changes

zee-bit added 5 commits August 11, 2021 22:54

neiljp force-pushed the renaming-no-topic branch from 7057718 to 4150981 Compare August 12, 2021 06:22

neiljp merged commit 84a265f into zulip:main Aug 12, 2021

neiljp removed the PR needs review PR requires feedback to proceed label Aug 12, 2021

neiljp added this to the Next Release milestone Aug 12, 2021

neiljp added the missing feature: user A missing feature for all users, present in another Zulip client label Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boxes: Add support for renaming (no topic) messages. #946

boxes: Add support for renaming (no topic) messages. #946

zee-bit commented Mar 12, 2021 •

edited

Loading

Ezio-Sarthak left a comment •

edited

Loading

zee-bit commented Mar 29, 2021 •

edited

Loading

neiljp left a comment

neiljp May 16, 2021

zee-bit May 18, 2021

zee-bit commented May 22, 2021

zee-bit Jul 4, 2021 •

edited

Loading

zee-bit Jul 4, 2021 •

edited

Loading

neiljp left a comment

neiljp Jul 5, 2021

zee-bit commented Jul 6, 2021

neiljp left a comment

neiljp Jul 6, 2021

zee-bit Jul 6, 2021 •

edited

Loading

neiljp Jul 6, 2021

neiljp Jul 6, 2021

zee-bit Jul 6, 2021 •

edited

Loading

zee-bit Jul 7, 2021

neiljp Jul 29, 2021

neiljp left a comment

neiljp Jul 29, 2021

zee-bit Aug 1, 2021 •

edited

Loading

neiljp Aug 12, 2021

neiljp Jul 29, 2021

neiljp Aug 12, 2021

neiljp Aug 12, 2021

neiljp commented Aug 12, 2021

neiljp commented Aug 12, 2021

		if message_fixture['type'] == 'private':
		to_vary_in_each_message['subject'] = ''

boxes: Add support for renaming (no topic) messages. #946

boxes: Add support for renaming (no topic) messages. #946

Conversation

zee-bit commented Mar 12, 2021 • edited Loading

Ezio-Sarthak left a comment • edited Loading

Choose a reason for hiding this comment

zee-bit commented Mar 29, 2021 • edited Loading

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit commented May 22, 2021

zee-bit Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

zee-bit Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit commented Jul 6, 2021

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Aug 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Aug 12, 2021

neiljp commented Aug 12, 2021

zee-bit commented Mar 12, 2021 •

edited

Loading

Ezio-Sarthak left a comment •

edited

Loading

zee-bit commented Mar 29, 2021 •

edited

Loading

zee-bit Jul 4, 2021 •

edited

Loading

zee-bit Jul 4, 2021 •

edited

Loading

zee-bit Jul 6, 2021 •

edited

Loading

zee-bit Jul 6, 2021 •

edited

Loading

zee-bit Aug 1, 2021 •

edited

Loading