TabbycatDebate / tabbycat

Debating tournament tabulation software for British Parliamentary and a variety of two-team parliamentary formats
https://tabbycat.readthedocs.io/
GNU Affero General Public License v3.0
245 stars 827 forks source link

Draw generator fails when no metrics are provided #1108

Closed ArthurPaiva closed 5 years ago

ArthurPaiva commented 5 years ago

I hit an internal server error while trying to load this URL: https://v-potiguar.herokuapp.com/vpotiguar2019/admin/draw/round/4/create/

Please describe what you were doing when the error happened, along with any other useful information:

Whenever try to generate a draw to round 4 on my tournment, this error displays: error

tienne-B commented 5 years ago

That is not really enough information to determine what is the problem. However, your site did send an error report, so I'll be examining through that (BACKEND-1QA).

Do you have team metrics set? Go to https://v-potiguar.herokuapp.com/vpotiguar2019/admin/options/standings/ and put "Wins" under "Team standings precedence". Or was it already set?

ArthurPaiva commented 5 years ago

For some reason I can't explain, now I can't even log in to TabbyCat as admin; it keeps saying that username and password does not match. As if it weren't enough, when I try to reset my password by sending an e-mail of that type to me, the same error [(Internal Server Error (500)] occurs. 1123123

tienne-B commented 5 years ago

Try going to the Heroku dashboard ( https://dashboard.heroku.com/apps/sucdi-tabs?web-console=v-potiguar ) and create a new user.

Click on the "More" dropdown (top-right) and select "Run Console". From there, type dj createsuperuser. It'll then ask for a new username and password. Once created, try logging in as the new account.

ArthurPaiva commented 5 years ago

I created the SU as you said and then reached to standins and put 'Wins'. It wasn't set. What should I do now? Because I still can't create the draw for Round 4, as I said before.

ArthurPaiva commented 5 years ago

GOOD NEWS! It has returned to work! So much thanks for the helping!

tienne-B commented 5 years ago

Did you save the preferences? You should be able to create the draw for round 4 now...

Oh great!

czlee commented 5 years ago

Thanks for the report @ArthurPaiva, and thanks for diagnosing it @tienne-B! Would I be understanding it right if I said that the issue seems to be that the system crashes when the team standings metrics are blank? If so, we should probably do something more graceful than throw a 500 in this circumstance, right?

ArthurPaiva commented 5 years ago

Yeah, exactly, @czlee. And for what's worth, I agree with you. xD

tienne-B commented 5 years ago

A warning could be placed on the availabilities page if power-pairing without metrics, or use wins as the default if the preference is not set, with a notice to that effect in the view draw page.

czlee commented 5 years ago

Hmm, the preference form shouldn't even be permitting it to save at all if the metrics are blank…

czlee commented 5 years ago

Oh damn okay, so I had to make allow_empty = True so that the "---------" option will show up. But as a consequence, it permits them all to be submitted empty, which isn't great.

This means there are two distinct issues:

czlee commented 5 years ago

Related: #749

Apparently I once put in a fix to protect against a similar situation… when there's only one metric in the precedence.

czlee commented 5 years ago

Actually, having thought about it more, there's no intrinsic reason Tabbycat should just refuse to run if the tab director didn't specify any team metrics. Everything in principle can still run, it'll just be very boring: everyone will trivially have exactly the rank.

Or should we just enforce it to make our lives simpler? Thoughts, @tienne-B @philipbelesky?

philipbelesky commented 5 years ago

Um I think that it’s probably best to try and enforce it within the form as that is probably the best time for a user to understand the behaviour.

Ideally it should also run if that behaviour is somehow set, but maybe display a warning highlighting why the rankings will look wrong.

czlee commented 5 years ago

Oh, sorry, my phrasing was poor. Obviously we should do something on the form. What I mean is: Enforce, or warn only?

To elaborate: "Enforce" means refuse to save preferences with a blank standings precedence, and raise a StandingsError to communicate to user why the standings/draw couldn't be generated. "Warn only" means save preferences but warn the user about the consequences, and generate the trivial standings/draw but with a message explaining why it might look weird.

I think we should be consistent between the preferences form and the draw/standings pages, i.e. either preference validation is enforced and it raises a (graceful) error if bad, or warnings appear in both places.

Basically the question whether we feel strongly enough about this to cut the option off from users entirely (given that our general default is to let users shoot themselves in the foot if they have good reason for doing so, but warn them about what they're doing).

philipbelesky commented 5 years ago

Hmm, I would lean on the side of enforce in the absence of a valid use case. We have a similar safeguard in place for the teams_in_debate and pre-adj ballots preference combination.

czlee commented 5 years ago

Potential use cases:

  1. Allowing a summary display of wins/losses in debates (because that's what the team tab, minus the last few columns, is), without implying an official ranking among teams (because of some quirk with a tournament format).
  2. "Hacking" a BP draw that is mostly random (not power-paired), but still runs position balance. (Not officially supported, but could be done in this way.)

Whether these are "valid" is… well, open to argument, I guess.

(The teams_in_debate+per-adj combination isn't really analogous: there's no way for us to code around that without building an entire system for dealing with voting ballots in BP. Paraphrased, that one's not just, "it will do something stupid by default", but, "what does that even mean?!")

czlee commented 5 years ago

Okay, with 6f3e8dc80131736cd3120eeffc265e33935d1a30 I've pretty much just made it so it works fine with no metrics, I guess (while simplifying some related code). I think this means I'd now lean towards warning only.