Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
neiljp opened this issue Oct 16, 2020 · 13 comments
Closed

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

neiljp opened this issue Oct 16, 2020 · 13 comments
Assignees
Labels
area: event handling How events from the server are responded to enhancement New feature or request good first issue Good for newcomers PR needs review PR requires feedback to proceed
Milestone

Comments

@neiljp
Copy link
Collaborator

neiljp commented Oct 16, 2020

We have preliminary support for realm emoji (custom emoji specific to an organization) via #710, but this has a few caveats (though perhaps minor, depending on the frequency of adding custom emoji vs each ZT session):

  • We don't update the realm emoji ZT is aware of, using events available from the server
  • We fetch using an API call after initial register, so even if we had updates, we might miss them between the two points - we should really just grab them out of the initial register() call (initial_data) instead

Working on this issue is a fairly good introduction to how the Zulip events API works. You'll likely want to review the events in the zulip documentation and existing code in model.py, which is where event handling occurs.

@neiljp neiljp added enhancement New feature or request good first issue Good for newcomers area: event handling How events from the server are responded to labels Oct 16, 2020
@mkp6781
Copy link
Contributor

mkp6781 commented Nov 12, 2020

I am new to the codebase and this is my first issue.
From what I have understood, these are the changes I intend to make:

  • add "realm_emoji" to fetch_event_types list
  • change line 148 in __init__ models.py from custom_emoji_data = self.fetch_custom_emojis() to
    custom_emoji_data = self.initial_data['realm_emoji']

When a client starts up, a call to _update_initial_data()is made which in turn calls _register_desired_events() Here the list of events in fetch_event_types() gets registered to the event queue. Finally the initial_data is updated with values in the returned response .
Please correct me if I've gone wrong.

@neiljp
Copy link
Collaborator Author

neiljp commented Nov 12, 2020

@mkp6781 What you've described should replace the existing code and is related to the first part of this issue (title - the second bullet).

The second part (of title, the first bullet) - "update using events" - requires an event handler for that event type to update them as emoji are added and removed on the server, so that the application retains a current list.

@mkp6781
Copy link
Contributor

mkp6781 commented Nov 13, 2020

 def _handle_update_emoji_event(self, event: Event) -> None:
        """
        Handle update of emoji
        """
        realm_emojis = event['realm_emoji']
        self.active_emoji_data.update(realm_emojis)

This is the event handler code. However when I run pytest, it gives me a 'key error' saying no key 'realm_emoji'. Should I make any more changes to pass the tests?

@neiljp
Copy link
Collaborator Author

neiljp commented Nov 13, 2020

The Event type likely doesn't have that key at this point. You also should check whether it should have that key, based on the different types of events available from the Zulip API.

https://zulip.com/api/get-events#realm_emoji-update

You're just quoting a function here so I'm not sure what else you've written, but note that this method won't be triggered unless you map the function to the name of the event type, as with the other event handlers.

Also note that you likely need to adjust the event data to understand what change to make to the emoji data.

@mkp6781
Copy link
Contributor

mkp6781 commented Nov 14, 2020

Pull request. #825

@mkp6781
Copy link
Contributor

mkp6781 commented Nov 16, 2020

Pull request #827

@Ezio-Sarthak
Copy link
Member

Hi @neiljp I am new to the codebase and this is my first issue. Could you please elaborate a bit what the issue is?

@mkp6781
Copy link
Contributor

mkp6781 commented Nov 27, 2020

I have updated the pr. #827
Please do have a look and suggest any more changes that may be necessary.

@mkp6781
Copy link
Contributor

mkp6781 commented Nov 29, 2020

@zulipbot claim

@neiljp
Copy link
Collaborator Author

neiljp commented Nov 29, 2020

@Ezio-Sarthak Someone is already working on this right now - have a look at other issues, explore the application and see what you think could be improved, and come and have a chat on chat.zulip.org in #zulip-terminal :)

@Ezio-Sarthak
Copy link
Member

@neiljp Yeah sure, thanks!

@mkp6781
Copy link
Contributor

mkp6781 commented Dec 10, 2020

@zulipbot add "PR needs review"

mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Jan 25, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Feb 9, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Feb 9, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Feb 9, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Feb 9, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Feb 13, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Feb 13, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Feb 15, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Feb 15, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Mar 4, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
@mkp6781
Copy link
Contributor

mkp6781 commented Mar 9, 2021

@zulipbot add "PR needs review"

@zulipbot zulipbot added the PR needs review PR requires feedback to proceed label Mar 9, 2021
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue Apr 1, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue May 2, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue May 3, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
mkp6781 added a commit to mkp6781/zulip-terminal that referenced this issue May 4, 2021
This commit adds event handler for update of realm emoji within a
ZT session. The generate_all_emoji_data() function unpacks the three
emoji dictionaries in the priority order given by
zulip extra emoji>realm emoji>unicode emoji

Tests added.

Fixes zulip#809
neiljp pushed a commit to mkp6781/zulip-terminal that referenced this issue May 5, 2021
This commit adds event handler for update of realm emoji within a
ZT session.

Tests added.

Fixes zulip#809.
@neiljp neiljp closed this as completed in 81070e6 May 5, 2021
@neiljp neiljp added this to the Next Release milestone May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: event handling How events from the server are responded to enhancement New feature or request good first issue Good for newcomers PR needs review PR requires feedback to proceed
Projects
None yet
Development

No branches or pull requests

4 participants