Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Fixes consistency issues in tips table & take_over. #1706

Merged
merged 10 commits into from Dec 4, 2013
Merged

Fixes consistency issues in tips table & take_over. #1706

merged 10 commits into from Dec 4, 2013

Conversation

zwn
Copy link
Contributor

@zwn zwn commented Dec 1, 2013

Fixes a bug in take_over that results in two entries in the tip table
with the same (tipper, tippee, mtime) but different amount.

Also removes unnecessary zero tips created in take_over when
neither of the users currently tips the tippee.

Further description in issue #1704.

@zwn
Fixes consistency issues in tips table & take_over.
9e63d4b 
Fixes a bug in take_over that results in two entries in the tip table
with the same (tipper, tippee, mtime) but different amount.

Also removes unnecessary zero tips created in take_over when
neither of the users currently tips the tippee.

Further description in issue #1704.
@zwn
Remove unnecessary zeroing of tips in take_over.
c9125fe
@chadwhitacre
Copy link
Contributor

I'm wrapping my head around this. I've got this PR locally and have added back the old queries in order to trigger a failing test with [gittip] $ foreman run -e env py.test test_participant.py -k self_tip:

        CONSOLIDATE_TIPS_RECEIVING = """

            INSERT INTO tips (ctime, tipper, tippee, amount)

                 SELECT min(ctime), tipper, %(live)s AS tippee, sum(amount)
                   FROM (   SELECT DISTINCT ON (tipper, tippee)
                                   ctime, tipper, tippee, amount
                              FROM tips
                          ORDER BY tipper, tippee, mtime DESC
                         ) AS unique_tips

                  WHERE (tippee=%(live)s OR tippee=%(dead)s)
                AND NOT (tipper=%(live)s AND tippee=%(dead)s)
                AND NOT (tipper=%(live)s)

-- New query:
--                WHERE (tippee = %(dead)s OR tippee = %(live)s)
--              AND NOT (tipper = %(dead)s OR tipper = %(live)s)
--                  AND amount > 0

               GROUP BY tipper

        """

        CONSOLIDATE_TIPS_GIVING = """

            INSERT INTO tips (ctime, tipper, tippee, amount)

                 SELECT min(ctime), %(live)s AS tipper, tippee, sum(amount)
                   FROM (   SELECT DISTINCT ON (tipper, tippee)
                                   ctime, tipper, tippee, amount
                              FROM tips
                          ORDER BY tipper, tippee, mtime DESC
                         ) AS unique_tips

                  WHERE (tipper=%(live)s OR tipper=%(dead)s)
                AND NOT (tipper=%(live)s AND tippee=%(dead)s)
                AND NOT (tippee=%(live)s)

-- New query:
--                WHERE (tipper = %(dead)s OR tipper = %(live)s)
--              AND NOT (tippee = %(dead)s OR tippee = %(live)s)
--                  AND amount > 0

               GROUP BY tippee

        """

@chadwhitacre
Copy link
Contributor

Here's the show_table output for test_take_over_self_test:

======================================tips======================================
id   | ctime                            | mtime                            | tipper | tippee | amount |
1612 | 2013-12-02 18:50:27.044107+00:00 | 2013-12-02 18:50:27.044107+00:00 | alice  | bob    | 1.00   |

==================================absorptions===================================

======================================tips======================================
id   | ctime                            | mtime                            | tipper       | tippee | amount |
1612 | 2013-12-02 18:50:27.044107+00:00 | 2013-12-02 18:50:27.044107+00:00 | a77f511c5172 | bob    | 1.00   |
1613 | 2013-12-02 18:50:27.044107+00:00 | 2013-12-02 18:50:27.048469+00:00 | a77f511c5172 | bob    | 1.00   |
1614 | 2013-12-02 18:50:27.044107+00:00 | 2013-12-02 18:50:27.048469+00:00 | a77f511c5172 | bob    | 0.00   |

==================================absorptions===================================
id  | timestamp                        | absorbed_was | absorbed_by | archived_as  |
414 | 2013-12-02 18:50:27.048469+00:00 | alice        | bob         | a77f511c5172 |

@chadwhitacre
Copy link
Contributor

So is it CONSOLIDATE_TIPS_GIVING or CONSOLIDATE_TIPS_RECEIVING that is resulting in the extra tips here? I hypothesize that it is CONSOLIDATE_TIPS_GIVING, because we are absorbing alice into bob, and the tips are coming from alice.

@chadwhitacre
Copy link
Contributor

False! I changed the commented-out part of the SQL, and it's the CONSOLIDATE_TIPS_RECEIVING query that is the one that triggers the bug in test_take_over_self_tip. Here's the good output:

======================================tips======================================
id   | ctime                            | mtime                            | tipper | tippee | amount | 
1628 | 2013-12-02 18:53:50.221421+00:00 | 2013-12-02 18:53:50.221421+00:00 | alice  | bob    | 1.00   | 

==================================absorptions===================================

======================================tips======================================
id   | ctime                            | mtime                            | tipper       | tippee | amount | 
1628 | 2013-12-02 18:53:50.221421+00:00 | 2013-12-02 18:53:50.221421+00:00 | cb95ada5972c | bob    | 1.00   | 
1629 | 2013-12-02 18:53:50.221421+00:00 | 2013-12-02 18:53:50.226327+00:00 | cb95ada5972c | bob    | 0.00   | 

==================================absorptions===================================
id  | timestamp                        | absorbed_was | absorbed_by | archived_as  | 
420 | 2013-12-02 18:53:50.226327+00:00 | alice        | bob         | cb95ada5972c |

@chadwhitacre
Copy link
Contributor

Bad:

======================================tips======================================
id   | ctime                            | mtime                            | tipper       | tippee | amount |
1612 | 2013-12-02 18:50:27.044107+00:00 | 2013-12-02 18:50:27.044107+00:00 | a77f511c5172 | bob    | 1.00   |
1613 | 2013-12-02 18:50:27.044107+00:00 | 2013-12-02 18:50:27.048469+00:00 | a77f511c5172 | bob    | 1.00   |
1614 | 2013-12-02 18:50:27.044107+00:00 | 2013-12-02 18:50:27.048469+00:00 | a77f511c5172 | bob    | 0.00   |

Good:

======================================tips======================================
id   | ctime                            | mtime                            | tipper       | tippee | amount | 
1628 | 2013-12-02 18:53:50.221421+00:00 | 2013-12-02 18:53:50.221421+00:00 | cb95ada5972c | bob    | 1.00   | 
1629 | 2013-12-02 18:53:50.221421+00:00 | 2013-12-02 18:53:50.226327+00:00 | cb95ada5972c | bob    | 0.00   |

@chadwhitacre
Copy link
Contributor

We have separate queries for ZERO_OUT_OLD_TIPS_{RECEIVING,GIVING}, and that's where the second tip comes from in the good case above.

@chadwhitacre
Copy link
Contributor

I'm not sure that CONSOLIDATE_TIPS_GIVING and CONSOLIDATE_TIPS_RECEIVING aren't named backwards. The way both work is that we select a set of tips from the tips table, and then insert new tips into the tips table for each of those, varying the mtime of the new tips.

@chadwhitacre
Copy link
Contributor

But in CONSOLIDATE_TIPS_RECEIVING we SELECT ... %(live)s AS tippee, and in CONSOLIDATE_TIPS_GIVING we SELECT ... %(live)s AS tipper. That means that the new live username (the absorbing user, so if bob absorbs alice then live is bob) is the tippee for the RECEIVING case and tipper for the GIVING case. Wouldn't we expect the opposite?

@chadwhitacre
Copy link
Contributor

That's my tentative explanation for why my hypothesis at #1706 (comment) was wrong.

@chadwhitacre
Copy link
Contributor

Wouldn't we expect the opposite?

No (yes?). "Tips giving" means bob is the tipper, "receiving" means he is the tippee.

chadwhitacre
chadwhitacre reviewed Dec 2, 2013
View reviewed changes
gittip/models/_mixin_elsewhere.py
@@ -213,9 +241,14 @@ def take_over(self, account_elsewhere, have_confirmation=False):

INSERT INTO tips (ctime, tipper, tippee, amount)

SELECT DISTINCT ON (tippee) ctime, tipper, tippee, 0 AS amount
FROM tips
WHERE tipper=%s
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm ... these also changed, I guess.

chadwhitacre added a commit that referenced this pull request Dec 4, 2013
@chadwhitacre
Merge pull request #1706 from gittip/issue-1704
791bafa 
Fixes consistency issues in tips table & take_over.
@chadwhitacre chadwhitacre merged commit 791bafa into master Dec 4, 2013
@chadwhitacre chadwhitacre deleted the issue-1704 branch December 4, 2013 21:47
@chadwhitacre chadwhitacre mentioned this pull request Dec 4, 2013
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@zwn @chadwhitacre