bugfix: Fix topic editing if topic unchanged #1056
Conversation
There was a problem hiding this comment.
@neiljp Thanks for pushing a patch! It seems to work well locally. 👍
Re the state data structure, fetching individual items from a tuple (using indices) doesn't tell much about what is being extracted out of the tuple. What do you think about a dict instead?
I have left a minor comment regarding a FIXME comment.
zulipterminal/ui_tools/boxes.py
Outdated
| @@ -539,10 +539,15 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]: | |||
| topic = self.title_write_box.edit_text | |||
|
|
|||
| if self.msg_edit_state is not None: | |||
| if topic == self.msg_edit_state[1]: # FIXME need strip? | |||
There was a problem hiding this comment.
For a topic change from "Test" to "Test ", it says invalid prop mode. I think we need to strip here.
The id is now within a namedtuple, which will be extended in future. Also explicitly update checks for the value versus None, rather than using truthiness. Tests amended.
neiljp
force-pushed
the
2021-06-24-bugfix-edit-content-only
branch
from
c9173ee to
2fd6254
Compare
|
Just returned to this after my time away as it blocks our use on czo with simple editing. @preetmishra Just pushed an updated version addressing your points:
The namedtuple change actually found a minor test change that was missing too, so thanks for the suggestion to avoid indices (even if internal to the WriteBox) |
There was a problem hiding this comment.
@neiljp Thanks for pushing the updates! 👍
The PR doesn't work currently but that's due to a small mishap (see my in-line comment). Other than that, the changes look good to me.
NamedTuple is even better with strict checking. Thanks for moving ahead with it. I second your decision regarding only using a trimmed topic name for equality and sending the topic as is until we have a guideline from the server.
zulipterminal/ui_tools/boxes.py
Outdated
| args = dict( | ||
| message_id=self.msg_edit_state.message_id, | ||
| topic=topic, | ||
| topic=topic, # NOTE: Send untrimmed topic always for now | ||
| propagate_mode=self.edit_mode_button.mode, |
There was a problem hiding this comment.
This should be propagate_mode=propagate_mode.
The intent of the edit/update message API has recently been clarified that if the topic is unchanged (even if sent) that the propagate_mode should be set to change_one. This was previously not enforced by the server but was changed to do so in zulip/zulip@e231a03 Since our propagate_mode defaults to change_later via the UI element, the above change to the server leads to errors upon attempting content-only edits. To resolve this, the original topic is added to the new tuple form of msg_edit_state, allowing detection of whether the topic has changed and setting of propagate_mode as appropriate. The decision to use change_one is dependent upon the trimmed new topic since otherwise the edit can confusingly fail since trimming is checked on the server end and results in the same propagate error message. Tests updated.
neiljp
force-pushed
the
2021-06-24-bugfix-edit-content-only
branch
from
2fd6254 to
415aeb5
Compare
|
@preetmishra Thanks for the reviews! Merging this now so I can get back to editing messages! 🎉 |
Recent work in resolved/unresolved topics has caused a change in the server (not yet in a release), where it more strictly enforces that if a topic is provided but unchanged in stream message editing/updating, then the propagate mode must be set to change_one - otherwise the edit does not proceed.
This PR works around this and has scope to be expanded towards resolving part or all of #1014.
The need for this was discussed in #design>closing/solving a topic (#11154).
I've not added tests for this change in behavior (yet, at least) - we don't seem to have tests for the existing behavior.