enhancement: Introduces application-wide time format setting. #917
Conversation
zulipbot
added
size: L
There was a problem hiding this comment.
@mkp6781 Thanks for following up on this after the refactor in the other PR 👍
This looks broadly good in practice, though I did note that with the year included the am/pm drops onto another line? (eg. look at messages before/after new-year 2020/2021)
tests/model/test_model.py
Outdated
| @pytest.mark.parametrize('event, twenty_four_hr_format', [ | ||
| ({'setting_name': 'twenty_four_hour_time', | ||
| 'setting': True}, False), | ||
| ], ids=['twenty_four_hour_time']) | ||
| def test__handle_update_display_settings(self, mocker, model, | ||
| twenty_four_hr_format, | ||
| event): | ||
| event['type'] = 'update_display_settings' | ||
|
|
||
| # Test for change in time format | ||
| if event['setting_name'] == 'twenty_four_hour_time': |
There was a problem hiding this comment.
This is a test for only the 24-hour update, and it's likely best to keep it at that - name it that, and if we add other settings event updates then we can write separate tests as we need them, or adapt this one. Those extra parameters are just one set, which you could put as a dict in the body of the test.
You're only testing False -> True? A lot of those parameters are constants once you focus on just this part of the event, and you can focus on the values of the settings.
There was a problem hiding this comment.
You're only testing False -> True? A lot of those parameters are constants once you focus on just this part of the event, and you can focus on the values of the settings.
Should I add tests for checking whether create_msg_box_list is being called and a separate test for formatted_local_time?
There was a problem hiding this comment.
Both in the same test seems OK for now. I'll look again in the next iteration.
neiljp
added
the
PR awaiting update
neiljp
removed
the
further discussion required
mkp6781
force-pushed
the
Application_wide_time_format
branch
from
0b939ed
to
c94e8b4
Compare
|
@zulipbot add "PR needs review" |
I missed looking into this part. I'll make the change along with next iteration. |
There was a problem hiding this comment.
@mkp6781 Just to let you know that the conflict is related to the year-related code I just merged from #930, which should also help clarify the comment from the last review re strftime. I left some comments, but this looks fairly ready to go if we can resolve these points well and get the missed points from the last review.
Initial time format settings are fetched during initial register call. Event handler enables update of this setting as events are recieved from the server. Tests added. Fixes zulip#770
mkp6781
force-pushed
the
Application_wide_time_format
branch
from
c94e8b4
to
19a2b4a
Compare
|
@mkp6781 Thanks for working on the server-related part of this after the refactoring PR 👍 I just manually merged this with some minor test and naming changes for consistency as 0608209 🎉 For reference, this would also have been fine as a "use initial data" commit followed by an "update based on events" commit - this PR should be simple enough that we've not overlooked anything that may have been clearer in that form 👍 |
Initial time format settings are fetched during initial register call.
Event handler enables update of this setting as events are recieved
from the server.
Fixes #770