model: Fetch realm emoji using events #827

mkp6781 · 2020-11-16T15:01:56Z

Continues work on #825.
Processed initial_data['realm_emoji] and set custom_emoji_data according to the schema given by NamedEmojiData.
Similar processing for realm_emoji_update before updating active_emoji_data in _handle_update_emoji_event.
Also, the deactivated realm emojis are removed from active_emoji_data.

neiljp

@mkp6781 Thanks for working on this, it looks like this is on the right path 👍

Did you open this to avoid having the other PR based on your master branch?

We may need special behavior with the zulip emoji - I'm not sure if that can be replaced and if we need to handle that specially locally or not.

The tests are running - and failing right now. This looks like only one or two issues, so you might find this easy to fix, either via travis or running pytest locally (see the pytest tips section).

It would be great if you could write some tests for this - these events aren't really that common to test manually on a normal server.

zulipterminal/model.py

mkp6781 · 2020-11-25T06:50:58Z

I have fixed the tests by adding realm_emoji to the initial_data in conftest.py.

It would be great if you could write some tests for this - these events aren't really that common to test manually on a normal server.

I am working on writing the test for _handle_update_emoji_event. Will update the PR soon.

neiljp

@mkp6781 Thanks for the update - you can just add a note here requesting review, or in principle adjusting the labels (if zulipbot is working). This feedback is a little longer due to giving some ideas about commit structure. I've only reviewed the net changes, and will review in more detail in a later iteration.

You have 6 commits now, but myself and other contributors don't really need to see the intermediate steps you're taking (unless you want to discuss those intermediate stages while working on it), particularly if you're adding/removing debug statements or correcting some other change from a previous commit. Given you have the same commit title for each commit, did you mean to have them all as one commit? If so you may want to investigate --amend in future. However, there is nothing wrong with multiple commits with temporary names that you can later bring together - it's easier to squash commits together than split them apart, generally.

For now, in terms of adjusting multiple commits, I'd recommend an interactive rebase to adjust your commits to tell a "story" of the changes you're making and why. As a pointer here, it'd be useful to have commits which make minimum changes to achieve:

migrate from using fetch to using the value possible in the initial_data
add event handling

After each commit linting/tests should pass, so the tests can be updated and extended for any new functionality in each commit.

You could also consider separate additional commits to refactor the code, to prepare for the related change, such as generalizing the name/parameterization of the emoji code.

If commits are functionally isolated in a sequence like this, this also means that independent groups of commits can sometimes be merged separately first, from the same PR, which can make discussion of the later commits and functionality easier.

tests/model/test_model.py

zulipterminal/model.py

mkp6781 · 2020-11-29T03:53:56Z

@zulipbot claim

neiljp · 2020-11-29T18:39:04Z

@mkp6781 I've assigned the issue to you manually for now, since zulipbot appears to be unresponsive.

mkp6781 · 2020-12-05T02:57:06Z

Hi
@neiljp Should I make more changes to the tests to make sure that we can use process_realm_emoji function again to update active_emoji_data?

mkp6781 · 2020-12-10T06:18:18Z

@zulipbot add "PR needs review"

neiljp

@mkp6781 Thanks for your work on this! I've left some comments below - let me know if you need more guidance 👍

zulipterminal/model.py

mkp6781 · 2021-01-25T18:15:36Z

@zulipbot add "PR needs review"

mkp6781 · 2021-01-25T18:21:32Z

As a pointer here, it'd be useful to have commits which make minimum changes to achieve:
* migrate from using fetch to using the value possible in the initial_data

* add event handling
After each commit linting/tests should pass, so the tests can be updated and extended for any new functionality in each commit.

Thank you for the elaborate description regarding the commit structure. I have modified my commits accordingly. I have also made the changes mentioned.

mkp6781 · 2021-01-27T06:58:47Z

@zulipbot remove "PR needs update"

mkp6781 · 2021-03-04T18:01:55Z

@zulipbot add "PR needs review"

mkp6781 · 2021-04-01T18:32:36Z

@zulipbot remove "PR awaiting update"

neiljp

@mkp6781 This older PR mainly needs a rebase and could benefit from some cleaning up of the tests using fixtures - otherwise we're going to end up with separate emoji data unused or duplicated - but otherwise it looks good to go 👍

The only aspect that comes to mind is that I'm unsure if there are consequences to removing emoji (eg. modifying messages to remove them being used on old messages, or just limiting them on new ones?) - I'm not sure if this is documented in the API or elsewhere.

neiljp · 2021-05-01T05:10:21Z

tests/model/test_model.py

-        assert model.active_emoji_data == OrderedDict(sorted(
-            {**unicode_emojis, **custom_emojis, **zulip_emoji}.items(),
-            key=lambda e: e[0]
-        ))


This and the two asserts below it are related. Given you add more asserts here, you could keep this assert too?

We can use the zulip_emoji fixture now though.

tests/conftest.py

tests/model/test_model.py

zulipterminal/model.py

neiljp · 2021-05-01T06:44:38Z

tests/model/test_model.py

+            "realm_emoji": {
+                "100": {
+                    "id": "100",
+                    "name": "urwid",
+                    "deactivated": True
+                },
+                "2": {
+                    "id": "2",
+                    "name": "green_tick",
+                    "deactivated": False
+                },
+                "202020": {
+                    "id": "202020",
+                    "name": "joker",
+                    "deactivated": True
+                },
+                "39": {
+                    "id": "39",
+                    "name": "zulip",
+                    "deactivated": False
+                },
+            },


It's not clear reading just the test asserts, how they relate to the original asserts in the initial data. We could extend the comments of course, however:

Since we get the full set of emoji each time, to improve test clarity this could be the same original dict (fixture) as you might get in the initial_data (embedded in this response), but with one variation in each of a number of parametrize? This response itself can be built in the test from a template and an approach used like in other tests where we use a to_vary* parameter.

I'm not sure about implementing a template for the emoji data. But I have modified the test to use the realm_emoji fixture and the to_vary* parameter.

This response itself can be built in the test from a template

Could you explain this a bit more!

zulipterminal/model.py

zulipterminal/api_types.py

mkp6781 · 2021-05-02T19:50:20Z

I have rebased the PR and made changes to the tests. Maybe there are a few more changes to be made which can be done in the next iteration.

The only aspect that comes to mind is that I'm unsure if there are consequences to removing emoji (eg. modifying messages to remove them being used on old messages, or just limiting them on new ones?) - I'm not sure if this is documented in the API or elsewhere.

I will think about this tomorrow.

neiljp

@mkp6781 The test structure looks much better with these changes 👍 A few minor points to consider, but this is essentially ready :)

For the final point in my last review, I'm not sure what the webapp does, if anything, but I'd suggest that could be something to research for a follow-up. If you don't want to take it up straight away, please make sure to file an issue for

zulipterminal/model.py

neiljp · 2021-05-03T00:05:07Z

tests/model/test_model.py

+        if (
+            to_vary_in_realm_emoji['deactivated']
+            and (
+                emoji_name not in unicode_emojis
+                and emoji_name not in zulip_emoji
+            )
+        ):


This may be more clearly expressed by using an intermediate boolean variable which you can then use like if emoji_should_be_active: or similar. That then clearly expresses what the complex expression represents.

tests/model/test_model.py

tests/conftest.py

tests/model/test_model.py

zulipterminal/api_types.py

zulipterminal/model.py

mkp6781 · 2021-05-03T17:35:07Z

For the final point in my last review, I'm not sure what the webapp does, if anything, but I'd suggest that could be something to research for a follow-up. If you don't want to take it up straight away, please make sure to file an issue for

The webapp doesn't make any changes to old messages when a realm emoji is added/removed and ZT follows the same behaviour.

mkp6781 · 2021-05-04T05:12:28Z

@zulipbot add "PR needs review "

zulipbot · 2021-05-04T05:12:30Z

ERROR: Label "PR needs review " does not exist and was thus not added to this pull request.

mkp6781 · 2021-05-04T05:12:39Z

@zulipbot add "PR needs review"

The existing code to obtain active_emoji_data is used in the generate_all_emoji_data() function to create the dictionary containing all the active emojis whenever required. zulip_extra_emoji fixture also extracted. Tests added for new function.

Earlier, an API call to get_realm_emoji() was used to fetch changes to realm emoji. This commit allows the initial update of realm emoji directly from initial_data. New test fixtures for `realm_emojis` and `realm_emojis_data` are added. Tests for `generate_all_emoji_data` modified to check various cases: * Adding new realm emoji. * Removing deactivated realm emoji. * Restoring unicode emoji with same name as a deactivated realm emoji. * Replacing existing unicode emoji when realm emoji with same name is added. * Ensuring zulip emoji is always of type `zulip_extra_emoji`.

This commit adds event handler for update of realm emoji within a ZT session. Tests added. Fixes zulip#809.

Defined class RealmEmojiData to represent realm emoji data more precisely with each key having value of a specific type.

neiljp · 2021-05-05T16:11:48Z

@mkp6781 Just pushed a few minor changes and merging now 🎉

Ideas for follow-up:

We could tighten up the last event test to indicate the expected type rather than whether it'll be present by broadening the expectation from bool to Optional[unicode/realm/zulip]?
A useful refactor could be to extract the type in the latter into api_types as EmojiType?

mkp6781 mentioned this pull request Nov 16, 2020

Fetch realm emoji in initial register call & update using events #809

Closed

neiljp reviewed Nov 19, 2020

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Nov 19, 2020

neiljp reviewed Nov 28, 2020

View reviewed changes

tests/model/test_model.py Outdated Show resolved Hide resolved

tests/model/test_model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

mkp6781 force-pushed the issue_809 branch from b6e1a7d to 0419543 Compare December 2, 2020 09:29

neiljp 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 Dec 17, 2020

neiljp added the has conflicts label Jan 7, 2021

neiljp reviewed Jan 11, 2021

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

zulipterminal/model.py Outdated Show resolved Hide resolved

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 Jan 11, 2021

mkp6781 force-pushed the issue_809 branch from 0419543 to 20870b5 Compare January 25, 2021 17:04

zulipbot added size: XL [Automatic label added by zulipbot] and removed has conflicts labels Jan 25, 2021

zulipbot added the PR needs review PR requires feedback to proceed label Jan 25, 2021

zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jan 27, 2021

neiljp added this to the Release after next milestone Jan 28, 2021

Base automatically changed from master to main January 30, 2021 20:31

zulipbot added the has conflicts label Jan 30, 2021

mkp6781 force-pushed the issue_809 branch from 20870b5 to 428e67b Compare February 9, 2021 12:15

zulipbot removed the has conflicts label Feb 9, 2021

mkp6781 force-pushed the issue_809 branch from 428e67b to 20a1a1d Compare February 9, 2021 14:12

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 Feb 17, 2021

mkp6781 force-pushed the issue_809 branch from a216569 to 8bf9715 Compare March 4, 2021 18:01

zulipbot added the PR needs review PR requires feedback to proceed label Mar 4, 2021

mkp6781 force-pushed the issue_809 branch from 8bf9715 to f1fccf7 Compare April 1, 2021 18:32

zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Apr 1, 2021

Ezio-Sarthak mentioned this pull request Apr 12, 2021

TRACKING: Event handling improvements #993

Open

15 tasks

neiljp reviewed May 1, 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 1, 2021

mkp6781 force-pushed the issue_809 branch from f1fccf7 to e3f00fa Compare May 2, 2021 19:32

neiljp reviewed May 3, 2021

View reviewed changes

mkp6781 force-pushed the issue_809 branch from e3f00fa to 10e6c5a Compare May 3, 2021 15:52

mkp6781 force-pushed the issue_809 branch from 10e6c5a to d6affc2 Compare May 4, 2021 05:10

zulipbot added the PR needs review PR requires feedback to proceed label May 4, 2021

mkp6781 added 4 commits May 5, 2021 08:21

model: Add event handling for realm emoji.

34580b3

This commit adds event handler for update of realm emoji within a ZT session. Tests added. Fixes zulip#809.

refactor: api_types/model: TypedDict for realm emoji data.

b10fbbc

Defined class RealmEmojiData to represent realm emoji data more precisely with each key having value of a specific type.

neiljp force-pushed the issue_809 branch from d6affc2 to b10fbbc Compare May 5, 2021 15:51

neiljp merged commit d7e576e into zulip:main May 5, 2021

mkp6781 mentioned this pull request May 9, 2021

Realm emoji test update #1026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model: Fetch realm emoji using events #827

model: Fetch realm emoji using events #827

mkp6781 commented Nov 16, 2020

neiljp left a comment

mkp6781 commented Nov 25, 2020

neiljp left a comment

mkp6781 commented Nov 29, 2020 •

edited

Loading

neiljp commented Nov 29, 2020

mkp6781 commented Dec 5, 2020

mkp6781 commented Dec 10, 2020

neiljp left a comment

mkp6781 commented Jan 25, 2021

mkp6781 commented Jan 25, 2021

mkp6781 commented Jan 27, 2021

mkp6781 commented Mar 4, 2021

mkp6781 commented Apr 1, 2021

neiljp left a comment

neiljp May 1, 2021

neiljp May 2, 2021

neiljp May 1, 2021

mkp6781 May 2, 2021 •

edited

Loading

mkp6781 commented May 2, 2021

neiljp left a comment

neiljp May 3, 2021

mkp6781 commented May 3, 2021

mkp6781 commented May 4, 2021

zulipbot commented May 4, 2021

mkp6781 commented May 4, 2021

neiljp commented May 5, 2021

model: Fetch realm emoji using events #827

model: Fetch realm emoji using events #827

Conversation

mkp6781 commented Nov 16, 2020

neiljp left a comment

Choose a reason for hiding this comment

mkp6781 commented Nov 25, 2020

neiljp left a comment

Choose a reason for hiding this comment

mkp6781 commented Nov 29, 2020 • edited Loading

neiljp commented Nov 29, 2020

mkp6781 commented Dec 5, 2020

mkp6781 commented Dec 10, 2020

neiljp left a comment

Choose a reason for hiding this comment

mkp6781 commented Jan 25, 2021

mkp6781 commented Jan 25, 2021

mkp6781 commented Jan 27, 2021

mkp6781 commented Mar 4, 2021

mkp6781 commented Apr 1, 2021

neiljp left a comment

Choose a reason for hiding this comment

neiljp May 1, 2021

Choose a reason for hiding this comment

neiljp May 2, 2021

Choose a reason for hiding this comment

neiljp May 1, 2021

Choose a reason for hiding this comment

mkp6781 May 2, 2021 • edited Loading

Choose a reason for hiding this comment

mkp6781 commented May 2, 2021

neiljp left a comment

Choose a reason for hiding this comment

neiljp May 3, 2021

Choose a reason for hiding this comment

mkp6781 commented May 3, 2021

mkp6781 commented May 4, 2021

zulipbot commented May 4, 2021

mkp6781 commented May 4, 2021

neiljp commented May 5, 2021

mkp6781 commented Nov 29, 2020 •

edited

Loading

mkp6781 May 2, 2021 •

edited

Loading