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

Add Like activity and ability to send a like #254

Merged
merged 11 commits into from
Dec 3, 2018
Merged

Add Like activity and ability to send a like #254

merged 11 commits into from
Dec 3, 2018

Conversation

CianLR
Copy link
Member

@CianLR CianLR commented Dec 1, 2018

Connects to #217

Kind of chunky PR but it's a lot of boilerplate and tests for the servicer mostly.

Copy link
Contributor

@SailSlick SailSlick left a comment

Choose a reason for hiding this comment

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

How come you decided to go with the activity before the local version?

build_container/test.sh Outdated Show resolved Hide resolved
services/activities/like/main.py Show resolved Hide resolved
services/activities/like/send_like_servicer.py Outdated Show resolved Hide resolved
services/activities/like/send_like_servicer.py Outdated Show resolved Hide resolved
services/proto/like.proto Outdated Show resolved Hide resolved
@SailSlick
Copy link
Contributor

Also whats the story with c/sendlike

Copy link

@devoxel devoxel left a comment

Choose a reason for hiding this comment

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

Overall very nice code. Just that one bit about global representation of a page, we can talk about that in the next meeting.

services/activities/like/main.py Show resolved Hide resolved
services/activities/like/send_like_servicer.py Outdated Show resolved Hide resolved
@CianLR CianLR mentioned this pull request Dec 2, 2018
@CianLR
Copy link
Member Author

CianLR commented Dec 2, 2018

This has been updated 👍

c/sendlike was a version of this change where I moved the activities stuff into utils in the same commits. I decided it was too big and split it, did a new branch because everything was mashed together in the commits.

def _create_actor_object(self, liker_handle):
return {
'type': 'Person',
'host': self._hostname,
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell this isn't a valid actor, as it has no ID. Why are you sending actors like this and not like the existing services? Why not send an ID like {host}/@{handle} and parse it out at the other end?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's tru actually, changed 👍


message LikeDetails {
int64 article_id = 1;
string liker_handle = 3;
Copy link
Member

Choose a reason for hiding this comment

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

handle only refers to the bit before the @ (ie. bob) in [email protected]. Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's all that's really needed since a like will only be sent by a local user, I attach the hostname inside the service.

@CianLR CianLR merged commit 0130011 into master Dec 3, 2018
@SailSlick SailSlick deleted the c/send_l branch December 6, 2018 11:06
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.

4 participants