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

Bodhi only allows users to change their mind about karma once #1064

Closed
bowlofeggs opened this issue Oct 26, 2016 · 0 comments · Fixed by #1066

Comments

@bowlofeggs
Copy link
Member

commented Oct 26, 2016

Due to this block in the comment() method, if a user submits a karma, and then later submits the opposite karma, they will be unable to submit the first karma again:

            mycomments = [c.karma for c in self.comments if c.user.name == author][reset_index:]
            if karma not in mycomments:
                if karma == 1 and -1 in mycomments:
                    self.karma += 2
                    caveats.append({
                        'name': 'karma',
                        'description': 'Your karma standing was reversed.',
                    })
                elif karma == -1 and 1 in mycomments:
                    self.karma -= 2
                    caveats.append({
                        'name': 'karma',
                        'description': 'Your karma standing was reversed.',
                    })
                else:
                    self.karma += karma

This is an edge case for certain, but it is not outside the realm of realistic possibilities and so we should fix it.

@puiterwijk discovered this problem while testing #1018 in staging. Fortunately, the proposed refactor for #1018 to make karma dynamically calculated should also fix this issue as a side effect.

@bowlofeggs bowlofeggs self-assigned this Oct 26, 2016

bowlofeggs added a commit to bowlofeggs/bodhi that referenced this issue Oct 26, 2016
Drop the Update.karma column and replace it with a property.
Prior to this commit Update karma had been denormalized, but that
led to a few issues with keeping the karma column correctly in sync
with the Comments in an Update. In one case, a user was able to
double +1 an Update, causing the karma column to contain an
incorrect value. The code that managed the karma column was
complicated and it was determined that the denormalized column did
not provide a significant enough performance gain to justify the
complex code it takes to maintain correct values in the database.
Nearly all views on Updates need to load the associated Comments
from the database, so calculating the Update.karma property
dynamically will not introduce any new queries in most cases.

This commit drops the karma column and replaces it with two
properties. The first calculates the sums of positive and negative
karma separately, and the second uses the first to generate and
return the overall karma. The first property was created to replace
the has_negative_karma() method, which had an issue where it failed
to correctly filter out karma before karma reset events. Rather
than fix that method, it was determined that it would be simpler to
drop it and rework the caller to use the new detailed_karma()
property to determine if there is negative karma.

This commit also addresses a third issue with karma counting where
users who changed their mind more than once would no longer be able
to change the karma of an Update.

fixes fedora-infra#1018
fixes fedora-infra#1064
fixes fedora-infra#1065
bowlofeggs added a commit to bowlofeggs/bodhi that referenced this issue Oct 27, 2016
Drop the Update.karma column and replace it with a property.
Prior to this commit Update karma had been denormalized, but that
led to a few issues with keeping the karma column correctly in sync
with the Comments in an Update. In one case, a user was able to
double +1 an Update, causing the karma column to contain an
incorrect value. The code that managed the karma column was
complicated and it was determined that the denormalized column did
not provide a significant enough performance gain to justify the
complex code it takes to maintain correct values in the database.
Nearly all views on Updates need to load the associated Comments
from the database, so calculating the Update.karma property
dynamically will not introduce any new queries in most cases.

This commit drops the karma column and replaces it with two
properties. The first calculates the sums of positive and negative
karma separately, and the second uses the first to generate and
return the overall karma. The first property was created to replace
the has_negative_karma() method, which had an issue where it failed
to correctly filter out karma before karma reset events. Rather
than fix that method, it was determined that it would be simpler to
drop it and rework the caller to use the new _composite_karma()
property to determine if there is negative karma.

This commit also addresses a third issue with karma counting where
users who changed their mind more than once would no longer be able
to change the karma of an Update.

fixes fedora-infra#1018
fixes fedora-infra#1064
fixes fedora-infra#1065
bowlofeggs added a commit to bowlofeggs/bodhi that referenced this issue Oct 27, 2016
Drop the Update.karma column and replace it with a property.
Prior to this commit Update karma had been denormalized, but that
led to a few issues with keeping the karma column correctly in sync
with the Comments in an Update. In one case, a user was able to
double +1 an Update, causing the karma column to contain an
incorrect value. The code that managed the karma column was
complicated and it was determined that the denormalized column did
not provide a significant enough performance gain to justify the
complex code it takes to maintain correct values in the database.
Nearly all views on Updates need to load the associated Comments
from the database, so calculating the Update.karma property
dynamically will not introduce any new queries in most cases.

This commit drops the karma column and replaces it with two
properties. The first calculates the sums of positive and negative
karma separately, and the second uses the first to generate and
return the overall karma. The first property was created to replace
the has_negative_karma() method, which had an issue where it failed
to correctly filter out karma before karma reset events. Rather
than fix that method, it was determined that it would be simpler to
drop it and rework the caller to use the new _composite_karma()
property to determine if there is negative karma.

This commit also addresses a third issue with karma counting where
users who changed their mind more than once would no longer be able
to change the karma of an Update.

fixes fedora-infra#1018
fixes fedora-infra#1064
fixes fedora-infra#1065
bowlofeggs added a commit to bowlofeggs/bodhi that referenced this issue Oct 27, 2016
Drop the Update.karma column and replace it with a property.
Prior to this commit Update karma had been denormalized, but that
led to a few issues with keeping the karma column correctly in sync
with the Comments in an Update. In one case, a user was able to
double +1 an Update, causing the karma column to contain an
incorrect value. The code that managed the karma column was
complicated and it was determined that the denormalized column did
not provide a significant enough performance gain to justify the
complex code it takes to maintain correct values in the database.
Nearly all views on Updates need to load the associated Comments
from the database, so calculating the Update.karma property
dynamically will not introduce any new queries in most cases.

This commit drops the karma column and replaces it with two
properties. The first calculates the sums of positive and negative
karma separately, and the second uses the first to generate and
return the overall karma. The first property was created to replace
the has_negative_karma() method, which had an issue where it failed
to correctly filter out karma before karma reset events. Rather
than fix that method, it was determined that it would be simpler to
drop it and rework the caller to use the new _composite_karma()
property to determine if there is negative karma.

This commit also addresses a third issue with karma counting where
users who changed their mind more than once would no longer be able
to change the karma of an Update.

fixes fedora-infra#1018
fixes fedora-infra#1064
fixes fedora-infra#1065
amolkahat added a commit to amolkahat/bodhi that referenced this issue Feb 2, 2017
Drop the Update.karma column and replace it with a property.
Prior to this commit Update karma had been denormalized, but that
led to a few issues with keeping the karma column correctly in sync
with the Comments in an Update. In one case, a user was able to
double +1 an Update, causing the karma column to contain an
incorrect value. The code that managed the karma column was
complicated and it was determined that the denormalized column did
not provide a significant enough performance gain to justify the
complex code it takes to maintain correct values in the database.
Nearly all views on Updates need to load the associated Comments
from the database, so calculating the Update.karma property
dynamically will not introduce any new queries in most cases.

This commit drops the karma column and replaces it with two
properties. The first calculates the sums of positive and negative
karma separately, and the second uses the first to generate and
return the overall karma. The first property was created to replace
the has_negative_karma() method, which had an issue where it failed
to correctly filter out karma before karma reset events. Rather
than fix that method, it was determined that it would be simpler to
drop it and rework the caller to use the new _composite_karma()
property to determine if there is negative karma.

This commit also addresses a third issue with karma counting where
users who changed their mind more than once would no longer be able
to change the karma of an Update.

fixes fedora-infra#1018
fixes fedora-infra#1064
fixes fedora-infra#1065
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.