boxes: Add support for community topic edits. #793
Conversation
Tests amended.
kaustubh-nair
force-pushed
the
community_editing_v1
branch
from
a9225f2 to
b836caa
Compare
|
Fixed tests and added feature level check. |
There was a problem hiding this comment.
@kaustubh-nair Functionally this seems mostly there, though I don't think it will support 2.1 properly?
While we do have a few cases of accessing the initial_data outside the model, I think the data and logic may be easier to follow if we place the check for editing in the model. That would make it easier to test separately from the UI. We have no cases outside the model which access the server_feature_level (except to display it), which emphasizes where we should put this. For example, we could have a method which returns what is editable ('all', 'topic', ''/None), an explanation (or simplified version of one), and possibly a time limit - based on whether a message is editable at the time of calling. We could consider something more complex to support time limits for topics and general editing separately, but we don't have the feature for that right now so let's stick with what's necessary :)
I'd have to check, but moving the code into the model may by default test with at least set the minimum feature level (None), which might cause this code to fail, assuming you don't override setting the property in the test of course - and if not, that may be a good thing to aim for, ideally with a way to customize other feature levels?
| key): | ||
| varied_message = dict(message_fixture, **to_vary_in_each_message) | ||
| size = (20,) | ||
| msg_box = MessageBox(varied_message, self.model, message_fixture) | ||
| msg_box.model.user_id = 1 | ||
| msg_box.model.server_feature_level = 11 |
There was a problem hiding this comment.
This is a good start, but I'd prefer to see tests at other feature levels, eg. like the op/operation case I added recently.
A test like that would pick up on server_feature_level being tested against None - which currently means this feature likely fails on Zulip 2.1.
neiljp
added
the
PR awaiting update
|
Heads up @kaustubh-nair, we just merged some commits that conflict with the changes your 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 |
No description provided.