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

Failing test and fix for running take_over twice. #1883

Merged
merged 3 commits into from
Jan 15, 2014

Conversation

zbynekwinkler
Copy link
Contributor

When user manages to run take_over twice with the same arguments,
tak_over creates an orphan. Fixes #617.

Update: Ready to review & merge. Required database update is included in branch.sql (deleting the orphans).

@zbynekwinkler
Copy link
Contributor Author

We have this code in take_over:

if we_already_have_that_kind_of_account:
    new_stub_username = reserve_a_random_username(cursor)
    cursor.run( "UPDATE elsewhere SET participant=%s "
                "WHERE platform=%s AND participant=%s"
                , (new_stub_username, platform, self.username)
                )
cursor.run( "UPDATE elsewhere SET participant=%s "
            "WHERE platform=%s AND user_id=%s"
            , (self.username, platform, user_id)
            )

If we are running the second time with the same parameters the first condition is true. We are also working with only one account elsewhere so the one we are moving away and the 'new' one is the same account. Thus orphan is created.

@seanlinsley
Copy link
Contributor

This seems to make sense to me. Is this ready to merge @zwn?

@zbynekwinkler
Copy link
Contributor Author

Yes, but I'd like @whit537 to see too before we merge it since he is the only one who groks further connections within this code.

@zbynekwinkler
Copy link
Contributor Author

@whit537 Soon we are going to assume that you have seen this and since you are not commenting it is fine for @seanlinsley to merge and for me to deploy 😉

seanlinsley added a commit that referenced this pull request Jan 15, 2014
Failing test and fix for running take_over twice.
@seanlinsley seanlinsley merged commit c5655e4 into master Jan 15, 2014
@seanlinsley seanlinsley deleted the take-over-creates-orphan branch January 15, 2014 00:17
@zbynekwinkler
Copy link
Contributor Author

Will deploy tonight, Central European Time.

@chadwhitacre
Copy link
Contributor

I reviewed this before deploying earlier today. !m @zwn

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

Successfully merging this pull request may close these issues.

someone managed to orphan their participant!
3 participants