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

votemanager issue #64

Closed
wants to merge 1 commit into from
Closed

votemanager issue #64

wants to merge 1 commit into from

Conversation

helios-ag
Copy link
Contributor

This commit is intended to discuss the following problem: how to restrict one vote per comment per user(voter)?
My solution (as for me) looks dirty, may be more experienced developers suggest a much better solution?
Also i have thoughts to make composite key (doctrine) combined from comment_id and voter_id, so a DBMS can restrict multiple votes per comment within same user.

@merk
Copy link
Member

merk commented Aug 22, 2011

Theoretically, the bundle can already support this,

https://github.com/FriendsOfSymfony/FOSCommentBundle/blob/master/Creator/DefaultVoteCreator.php#L58

If you define validation constraints for your Vote model, the validation will fail and the vote will not be created.

{
$comment->incrementScore($vote->getvalue());
$this->em->flush();
}
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. You flush conditionally, but this does not avoid the UnitOfWork being flushed by another part of the request.

The check should wrap the whole method if done this way.

Also, note that you don't follow the Symfony2 CS, and that the ODM version needs to behave the same way.

@Problematic
Copy link
Contributor

@merk what kind of validation constraints would you recommend to achieve this?

@merk
Copy link
Member

merk commented Sep 9, 2011

Potentially something like

<?php

use Symfony\Component\Validator\Constraints as Assert;
use Symfony\Bridge\Doctrine\Validator\Constraints as DoctrineAssert;
use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity
 * @DoctrineAssert\UniqueEntity({"comment", "user"})
 */
class Vote
{
// ...
}

Now, that isnt complete - the unique entity constraint should be on both the comment and user relationships but im not sure if it is supported or not.

Let me know if that works out for you. Unfortunately I'm on medical leave at the moment and I'm unable to spend much time debugging code.

@Problematic
Copy link
Contributor

That was my first thought, too, but I've been playing around with it for a bit and can't get it to behave properly. I'll see what I can cook up.

@Problematic
Copy link
Contributor

@helios-ag are you still working on this solution? If not, I may use it as inspiration if you don't mind. I opened #68 to use a model constraint because I thought it would be nice to have optional rate-limiting, but I've been thinking about it, and I can't think of a use-case where you wouldn't want to use rate limiting; it kind of defeats the point otherwise.

If you are still working on this, let us know how else we can help!

@merk
Copy link
Member

merk commented Nov 20, 2011

I would like to see a solution to this but do not have a need for it.

The best idea I've come up with is a validator provided by FOSCB which will use the VoteManager to check if the user has voted within a specific time.

It would work something like the Unique validator provided by FOSUB (https://github.com/FriendsOfSymfony/FOSUserBundle/blob/master/Validator/UniqueValidator.php)

@stof
Copy link
Member

stof commented Dec 18, 2011

@merk ping here

@merk
Copy link
Member

merk commented Jan 7, 2012

I am closing this pull request because it isnt the right solution.

It should be possible to overwrite the VoteCreator to perform specific checks if the validator component is unable to cope with them.

@merk merk closed this Jan 7, 2012
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.

None yet

4 participants