t2t2 / draft

MOVIES MOVIES MOVIES!
http://boxofficedraft.com/
MIT License
18 stars 8 forks source link

Plausible Bug: Removing a movie doesn't remove movie-player connection #26

Closed t2t2 closed 9 years ago

t2t2 commented 10 years ago

There might be a bug where if a player deletes a movie from the league it doesn't get removed from league_movie_user table

Test data:

SELECT * FROM `league_movie_user` WHERE `league_id` = '185'

id  league_id   movie_id    user_id
1374    185 106 137
1375    185 108 135
1376    185 107 505
1377    185 109 137
1378    185 110 135
1379    185 111 135
1380    185 113 136

SELECT * FROM `league_movie` WHERE `league_id` = '185'

id  league_id   movie_id    price   latest_earnings_id
5552    185 112 0   NULL
5553    185 113 17  NULL
5554    185 114 0   NULL
5555    185 115 0   NULL
5556    185 116 0   NULL
5557    185 117 0   NULL
5558    185 118 0   NULL
5559    185 119 0   NULL
5560    185 120 0   NULL
5561    185 121 0   NULL
5562    185 122 0   NULL
5563    185 123 0   NULL
5564    185 124 0   NULL
5565    185 125 0   NULL
5566    185 126 0   NULL
5567    185 127 0   NULL
5568    185 128 0   NULL
5569    185 129 0   NULL
5570    185 130 0   NULL
5571    185 131 0   NULL
5572    185 132 0   NULL
5573    185 133 0   NULL
5574    185 134 0   NULL
5575    185 135 0   NULL
6275    185 111 0   NULL
vincent404 commented 10 years ago

Huh. So it just hangs around in the DB, but doesn't show on the league page, correct?

t2t2 commented 10 years ago

It deletes the league <-> movie link, but not the league - movie - user link.

Unless there was some weirdass corruption happening on the database

rkuykendall commented 10 years ago

Hey. I thought I'd use this as an opportunity to get a little familiar with the code. Below is the snippet I think is responsible, from LeagueAdminController.php. Unfortunately, I read over quite a few times without finding anything wrong. The long if statement looks like the most error prone part, but since the movie is being removed from the league, the if statement should be working. As far as I can tell, there is nothing wrong with the for loop removing player movie league relationships.

if(Input::has('remove') && filter_var(Input::get('remove'), FILTER_VALIDATE_INT) !== false && $movie = $league->movies->find(Input::get('remove'))) {
  // Remove any possible attached players
  foreach ($movie->users as $user) {
    $query = DB::table('league_movie_user')->whereLeagueId($league->id)->whereMovieId($movie->id)->whereUserId($user->id);
    $query->delete();
    if(array_search($user->id, $update_players) === false) {
      $update_players[] = $user->id;
    }
  }
  // Remove movie
  $league->movies()->detach($movie->id);
  // Instead of adding it to changes count, have a seperate notification
  Notification::success('Movie '.e($movie->name).' has been removed from the league.');
}
t2t2 commented 10 years ago

One more report of something being weird: https://twitter.com/rclaybones/status/453247379408318465

vincent404 commented 10 years ago

That is strange. Honestly, this will be our real first season of heavy usage so we're bound to find bugs and weird oddities. — Sent from Mailbox for iPhone

On Mon, Apr 7, 2014 at 2:11 PM, t2t2 notifications@github.com wrote:

One more report of something being weird: https://twitter.com/rclaybones/status/453247379408318465

Reply to this email directly or view it on GitHub: https://github.com/t2t2/draft/issues/26#issuecomment-39770892

t2t2 commented 10 years ago

Looks like there might be a bug with some browsers submitting via delete buttons, does anyone e have an iPad to test with?

https://twitter.com/rclaybones/status/453300222761136129

vincent404 commented 10 years ago

I've got one. Let me know the things to recreate or if you've got a special link for tracking. — Sent from Mailbox for iPhone

On Mon, Apr 7, 2014 at 5:46 PM, t2t2 notifications@github.com wrote:

Looks like there might be a bug with some browsers submitting via delete buttons, does anyone e have an iPad to test with?

https://twitter.com/rclaybones/status/453300222761136129

Reply to this email directly or view it on GitHub: https://github.com/t2t2/draft/issues/26#issuecomment-39793204

t2t2 commented 10 years ago

Made a test case: http://t2t2.eu/test/draftr_form.php

And it looks like it's something that can happen in any browser if enter is used to submit the form

rkuykendall commented 10 years ago

I was able to fix it by adding this to the beginning of the form:

<input type="submit" value="Save" class="btn-primary btn" style="visibility: hidden;">

I submitted a pull request, but that was mostly just because I had never done one before and wanted to see how they worked. The code hasn't been tested on a running instance of draft. Let me know what you think.