Skip to content

WIP: Reaction frontend#2387

Closed
kracekumar wants to merge 4 commits into
zulip:masterfrom
kracekumar:reaction-frontend
Closed

WIP: Reaction frontend#2387
kracekumar wants to merge 4 commits into
zulip:masterfrom
kracekumar:reaction-frontend

Conversation

@kracekumar
Copy link
Copy Markdown
Contributor

This is tracking PR for #541. This is forked from #2245.

arpith and others added 2 commits November 21, 2016 15:09
This commit adds the following.

1. A reaction model that consists of a user, a message and an emoji
2. A reaction event that looks like:
    {
        'type': 'reaction',
	'message_id': 3,
	'emoji_name': 'doge',
	'user': {
	    'user_id': 1,
            'email': 'hamlet@zulip.com',
            'full_name': 'King Hamlet'
	}
    }
3. A new API endpoint, /reactions, that accepts POST requests to add a
reaction to a message
4. A migration to add the new model to the database
5. Tests that check that
   (a) Invalid requests cannot be made
   (b) The reaction event body contains all the info
   (c) The reaction event is sent to the appropriate users
@smarx
Copy link
Copy Markdown

smarx commented Nov 22, 2016

Automated message from Dropbox CLA bot

@kracekumar, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here.

@timabbott
Copy link
Copy Markdown
Member

@kracekumar I merged the initial emoji backend commit after fixing some things; you'll want to rebase, dropping the original backend commit from @arpith's branch. Let me know if you need any help.

Comment thread zerver/lib/actions.py
'full_name': user_profile.full_name}

event = {'type': 'reaction',
'action': 'add',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got added in master as 'op': 'add'; you should switch your code to use that when you rebase.

@timabbott
Copy link
Copy Markdown
Member

I took a quick look at your code for doing the fetching @kracekumar, and it looks like you've done a pretty reasonable job of achieving the main goal (not doing database queries in loops). You'll likely need to update several backend tests (tools/test-backend to see the errors) to increase the database query count by 1.

@kracekumar
Copy link
Copy Markdown
Contributor Author

@timabbott Today I will rebase to the latest master. Does it make sense to send get_old_messages fix alone as a separate PR?

@timabbott
Copy link
Copy Markdown
Member

Either a separate PR or a separate commit at the start of the branch; either way the goal will be to get that commit into shape and merge it next.

@qwo
Copy link
Copy Markdown

qwo commented Dec 6, 2016

@kracekumar you might want to close this PR since we pulled in all the logic required in my branch

@smarx
Copy link
Copy Markdown

smarx commented Dec 6, 2016

Automated message from Dropbox CLA bot

@kracekumar, thanks for signing the CLA!

@kracekumar
Copy link
Copy Markdown
Contributor Author

@stanzheng You're correct.

@kracekumar kracekumar closed this Dec 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants