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

Update.has_negative_karma() counts karma from before new or removed builds #1065

Closed
bowlofeggs opened this issue Oct 26, 2016 · 1 comment · Fixed by #1066

Comments

@bowlofeggs
Copy link
Member

commented Oct 26, 2016

Bodhi's karma counting code resets the karma to 0 when builds are added or removed from an Update, and a user's karma from before the reset is not supposed to be counted:

            # Take all comments since the previous karma reset
            reset_index = 0
            for i, comment in enumerate(self.comments):
                if (comment.user.name == u'bodhi' and ('New build' in
                    comment.text or 'Removed build' in comment.text)):
                        reset_index = i

Due to #1018 this code doesn't work quite as intended, but it also occurred to me that Update.has_negative_karma() does count karma that was given before karma reset events:

    def has_negative_karma(self):
        """Check for negative karma, Returns True if the update has negative karma"""
        feedback = defaultdict(int)
        for comment in self.comments:
            if not comment.anonymous:
                feedback[comment.user.name] = comment.karma
        for karma in feedback.values():
            if karma < 0:
                return True
        return False

This means that critical path updates that add or remove builds after receiving negative karma will still automatically get disabled when receiving new karma comments after the reset event because the old negative karma is still affecting the test. The old karma will not contribute to the Update's karma, however.

This is an edge case scenario, and fails in a "safe" way because the consequence is simply that autopush will get disabled for the Update. Therefore, I consider this a low priority issue.

@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2016

I am adding a new karma() method that counts the karma correctly in order to solve #1018. I think I will create a karma_details() method while I'm in there that returns the good and bad karma counts separately. Then karma() can just sum the two and has_negative_karma() can be removed in favor of just comparing the bad karma count returned by karma_details(). I'm killing three birds with this stone ☺

@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.