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

Allow authenticated user to vote/remove vote on card, add idMembersVoted to card #208

Merged
merged 4 commits into from
Aug 11, 2016

Conversation

brycemcd
Copy link
Contributor

@brycemcd brycemcd commented Aug 4, 2016

What

See #202 for more details. This PR adds support for adding/removing votes to a card. It also adds idMembersVoted to card attributes (though this last part is WIP)

How to test

Authenticate and execute the following code:

card = Trello::Card.find('id-of-card')
card.upvote
card.remove_upvote

# now do it and be obnoxious
card.upvote
card.upvote
card.upvote

card.remove_upvote
card.remove_upvote

Note

I'll be able to add idMembersVoted to the PR tomorrow (2016-08-04)

private def get_authenticated_user_id
mem = Member.from_response client.get('/members/me')
mem && mem.id
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, this is the wrong place to put this. Would adding a Member.me class method be a good idea? The concept of "me" exists in the API and is documented here: https://developers.trello.com/advanced-reference/member#get-1-members-idmember-or-username

#
# @return [Array<Trello::Member>]
def voters
Member.from_response client.get("/cards/#{id}/membersVoted")
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we also please have a spec for this method?

@brycemcd
Copy link
Contributor Author

brycemcd commented Aug 5, 2016

@joneslee85 this is ready for your review.

One defect I've noticed, and tried to solve, is that when the state of the object changes (like when an upvote occurs) the whole object is not refreshed. EX:

@card # => badges: {votes: 0}
@card.upvote # => badges: {votes: 1}
@card # => badges: {votes: 0}, should be votes: 1 

I attempted to solve this in a number of ways but realized that this behavior is consistent with other attributes (like labels). I'd expect consumers of this gem have established patterns of handling this. It felt like a defect to me though.

CI is failing at the moment because I'm using the private def foo syntax for that "me" method. I'll fix it up after I get some guidance on how best to handle that.

card.remove_upvote
end

it 'returns members that have voted for the card' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spec for Card#voters is here. Want it in its own context?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please

@runlevel5
Copy link
Collaborator

@brycemcd regarding the defect, It'd be ideal that we return a refreshed / updated clone of the card object. However this seems to be too out of scope. For the time being, can you please write a spec noting this defect? We could discuss in further how to rectify that issue.

@brycemcd brycemcd changed the title [WIP] Allow authenticated user to vote/remove vote on card, add idMembersVoted to card Allow authenticated user to vote/remove vote on card, add idMembersVoted to card Aug 9, 2016
@runlevel5
Copy link
Collaborator

@brycemcd let me know when it is good for another review round

@brycemcd
Copy link
Contributor Author

@joneslee85 It should be ready to go now (was waiting for CI to confirm tests pass). I'll get a new issue/PR opened for the defect I found in doing this work in the next day or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants