Refactor DebateResult and associated classes #1180

tienne-B · 2019-08-10T03:51:06Z

This branch is the work in refactoring DebateResult, Scoresheet, and ResultForm for technical debt and allowing for new types of results. See commit messages for more details.

Supersedes #1004

Goal:
Fixes #527
Fixes #643
Fixes #1003

- Removing obsolete comment from tables.py - Converted name of current iron parameter from camelCase

tabbycat/results/scoresheet.py

tienne-B · 2019-09-05T03:09:06Z

tabbycat/options/preferences.py

@@ -461,6 +461,21 @@ class BallotsPerDebateElimination(ChoicePreference):
    default = 'per-adj'


+@tournament_preferences_registry.register
+class BallotMustConfirmWinner(ChoicePreference):
+    help_text = _("Whether adjudicator(s) must select the winning team in their ballot, and how it should be treated. Note: Not supported in BP.")


I didn't really know how to word this preference...

Hmm, good question. I think what you've got for the help text above is good. I tend to think of the three below options as constraints on scores rather than on the winner, though. Like:

Do not require separate winner selection; infer from higher speaker score (no ties allowed)

High-point wins only: Require winner selection to verify that winning team scored higher

Tied-point wins allowed: Require winner selection to verify that winning team tied or scored higher

Low-point wins allowed: Selected winning teams wins, regardless of speaker scores

This commit adds a new table to the Results application to hold team scores by individual adjudicators (TeamScoreByAdj) as a counterpart to SpeakerScoreByAdj and TeamScore. Its use is primarily for by-adj elimination ballots. The uses are not yet implemented. It has these fields: - Ballot submission - Debate-Adjudicator - Debate-Team - Win? - Margin - Score The points field was not included as only two-team formats can use this table and the win column can serve the same role. The margin field was added as it could signal greater dissent from an adjudicator than another. A database migration has also been added to both create this model and to populate it with existing SpeakerScoreByAdj records. All of the current data in this table will be redundant with existing data. Some methods in other results admin classes have been removed as duplicating inherited methods.

This commit reduces the number of various DebateResult classes by making them agnostic to the number of teams. With that, the base class branches to "By Adjudicator" and "Consensus," with a mixin included for their subclasses to support scoring. For this, scoring is assigning speaker scores, which are then combined to create team scores. Teams may not be scored without speakers. This greatly simplifies the inheritance structure of DebateResult. The scoresheet selection for each type of DebateResult is in the DebateResult class. To support BP in this structure, the `winners()` attribute of scoresheets is now a list of winning sides. There is an assertion to prevent an incorrect number of winners though. Also, a specific scoresheet for BP Elimination has been created. No new options to access DebateResults without scores has been created in this patch. Tests are also updated to take the new structure, but no new tests have been added as of this commit. Finding the proper DebateResult class has been spun-off from the factory function.

As a consequence of the previous commit, uses of DebateResult in certain places needed to be updated. This commit updates its use for ballot notifications (using isinstance) and the dropping of "advancing" in favour of the generalization "winner(s)" in results. This commit does not update ballot forms, which will be dealt with separately.

This change makes the current forms usable with the new format of DebateResult, which will be refactored in another commit. It also marks the password label for translation and does some cleanup.

This commit adds fields for adjudicators to precise the winning team on their ballots through a drop-down after their scores, using side names. Checks have also been added to results to check for empty lists when finding the winner.

This commit creates a preference to activate the winning team selection drop-down in ballots, further enabling tied-points and low-point wins with their associated scoresheet classes. Validation has been added to the forms if the declared winner does not correspond to the form in high-point wins.

This commit refactors the result forms to be orthogonal to the Debate- Result classes. In that, mixins are used to distinguish forms using (or not) scores. The DebateResult class has also been added as a parameter for the forms with a correspondence dict between forms and results in views.py. A new preference allows for speaker scores to be omitted from ballots depending on the round stage.

This commit replaces all the logic to determine the result of a debate, such as the margin/splits, etc. with completing a DebateResult object and saving that. This commit refers to methods created as part of TabbycatDebate#1180.

# Conflicts: # tabbycat/results/result.py # tabbycat/tournaments/models.py

This commit creates API endpoints that can be used to get and create debate results. Result endpoints are under each debate (pairing), and can be filtered to just the confirmed ballots. When public, only the confirmed ballot is shown. The base for the results is in a new class ResultInfo, which takes a DebateResult object and transforms the methods to retrieve info into object attributes, that can then be read by DRF serializer fields. Serializer subclasses were created to mirror the new DebateInfo structure, but the save() methods bypass ResultInfo to set and save DebateResult directly.

This commit adds the schema definition for the Results endpoints to the OpenAPI schema.

# Conflicts: # tabbycat/api/serializers.py

czlee

Hello, this happens when I set it up on a standard Australs setup:

Traceback (most recent call last):
  File "/home/czlee/tabbycat/venv/lib/python3.8/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
[...]
  File "/home/czlee/tabbycat/venv/lib/python3.8/site-packages/django/forms/forms.py", line 402, in _clean_form
    cleaned_data = self.clean()
  File "/home/czlee/tabbycat/tabbycat/results/forms.py", line 514, in clean
    self.clean_scoresheet(cleaned_data)
  File "/home/czlee/tabbycat/tabbycat/results/forms.py", line 831, in clean_scoresheet
    high_point_declared = max(side_totals, key=lambda key: side_totals[key]) == cleaned_data[self._fieldname_declared_winner()]
TypeError: _fieldname_declared_winner() missing 1 required positional argument: 'adj'

czlee · 2020-10-05T22:00:53Z

tabbycat/options/preferences.py

@@ -461,6 +461,21 @@ class BallotsPerDebateElimination(ChoicePreference):
    default = 'per-adj'


+@tournament_preferences_registry.register
+class BallotMustConfirmWinner(ChoicePreference):
+    help_text = _("Whether adjudicator(s) must select the winning team in their ballot, and how it should be treated. Note: Not supported in BP.")


Hmm, good question. I think what you've got for the help text above is good. I tend to think of the three below options as constraints on scores rather than on the winner, though. Like:

Do not require separate winner selection; infer from higher speaker score (no ties allowed)

High-point wins only: Require winner selection to verify that winning team scored higher

Tied-point wins allowed: Require winner selection to verify that winning team tied or scored higher

Low-point wins allowed: Selected winning teams wins, regardless of speaker scores

czlee · 2020-10-06T03:02:41Z

Adding to review above:

I get this on standard Australs settings except with high-point win verification enabled, regardless of who I select as the winner:

Traceback (most recent call last):
  File "/home/czlee/tabbycat/venv/lib/python3.8/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
[...]
  File "/home/czlee/tabbycat/venv/lib/python3.8/site-packages/django/views/generic/edit.py", line 142, in post
    return self.form_valid(form)
  File "/home/czlee/tabbycat/tabbycat/results/views.py", line 309, in form_valid
    self.ballotsub = form.save()
  File "/home/czlee/tabbycat/tabbycat/results/forms.py", line 186, in save
    self.save_ballot()
  File "/home/czlee/tabbycat/tabbycat/results/forms.py", line 434, in save_ballot
    self.save_participant_fields(result)
  File "/home/czlee/tabbycat/tabbycat/results/forms.py", line 599, in save_participant_fields
    self.populate_result_with_scores(result)
  File "/home/czlee/tabbycat/tabbycat/results/forms.py", line 861, in populate_result_with_scores
    result.set_winners(adj, set([declared_winner]))
  File "/home/czlee/tabbycat/tabbycat/results/result.py", line 456, in set_winners
    self.scoresheets[adjudicator].set_declared_winners(winners)
AttributeError: 'HighPointWinsRequiredScoresheet' object has no attribute 'set_declared_winners'

I get this template error on standard Australs settings except with tied-point win verification enabled, and selecting the lower-scoring team to win (i.e. an invalid ballot):

KeyError at /australs24team/admin/results/debate/old/89/new/
'adj'

In template /home/czlee/tabbycat/tabbycat/templates/components/form-errors.html, error at line 7
  {% include "components/alert.html" with type="danger" extra=errors\|cut:'__all__' %}

[Not a showstopper:] I think we probably want to include "affirmative" and "negative" here, a lot of people find it easier to think of teams in terms of positions than in terms of team names. Either instead of, or as well as—I'm not sure which is better, but I think position names should be in there somewhere.

Also I would like to apologize again for taking so long to spin this up for a test run on my local system. I'm hoping that with the branch checked out and a test database for this branch set up, I can now do a quick sanity drive quickly next time to pick up any obvious things, before taking longer to do the thorough check.

czlee · 2020-10-06T03:03:33Z

Can I also pull @philipbelesky in to offer guidance on how this would fit in with the old/new results entry interfaces? This obviously requires changes to the result entry form; should we be (evenutally) modifying both forms or just one of them?

philipbelesky · 2020-10-06T03:14:48Z

I think just the old results entry form. I've come to the conclusion that if/when the new entry interface is finished it needs to post to an API endpoint rather than its current (flawed) approach of trying to wrap Vue around Django's forms. That change is pretty much a rewrite so I don't think the current state of the 'new' form needs to be kept up to date - if anything it could be removed.

- Renamed "adj" message parameter to "adjudicator" - Added team sides to declared winners dropdown - Don't set declared winners with high-point wins - Specify adj in voting validation declared winner

# Conflicts: # tabbycat/results/views.py

tienne-B · 2020-10-09T04:29:49Z

Fixed

czlee · 2020-10-13T05:26:43Z

Forms look better now, thanks! Will go over code/internals a bit later.

Don't put this in this PR (doesn't even need to be this release), but I wonder if we can optionally highlight low-point/tied-point wins in the results table.

czlee

A few minor things I noticed but looks good, will merge and if you want to touch anything up feel free to do so in a new commit pushed straight to develop.

tabbycat/results/scoresheet.py

czlee · 2020-10-16T03:16:07Z

tabbycat/results/result.py

-            logger.exception("Tried to set score, but scoresheet doesn't do scores. "
-                    "self.takes_scores is %s", self.takes_scores)
+    def get_winner(self, adjudicator):
+        return next(iter(self.scoresheets[adjudicator].winners()), None)


Is there an assumption here that there is at most one winner whenever this function is called? If so, it's probably better to include an assert statement that verifies that .winners() has at most one item, and then return that item using .pop(), rather than using next(iter(...)). That way if the condition fails, it fails fast, and also it'll be more explicit to read.

No, there may be either 0 or 1 winners, and if there are no winners, None should be returned. .pop() couldn't be used as it would mutate the set.

I said at most one winner 😉 as in, there can't be more than one, though there might be less than one. But good point re pop()—kinda dissatisfying that next(iter(...)) is the cleanest way otherwise, but it is what it is! I still think you want to assert assumptions to get it to fail fast though, or at least get the logger to throw an error at Sentry (if you think crashing due to the state is worse than ploughing ahead silently).

tabbycat/results/result.py

tabbycat/tournaments/views.py

czlee · 2020-12-27T08:08:43Z

Just noting a regression in #922 (see reopening comment there).

The get_speaker_name override in SpeakerScoreByAdjAdmin was removed in 92511f9 (#1180), which causes the speaker score by adj admin page to crash as the implementation in the parent class relies on the `speaker` attribute, which SpeakerScoreByAdjAdmin does not have.

This restores the line added in a29981d to make set_score() fail silently if the adjudicator is not on the panel. It was removed by the merger of #1180, specifically in a0eb1f7, which had been done long before a29981d but not been merged until it was merged with #1180 (i.e., it was a merge conflict whose resolution appears to have been overlooked). Fixes #922.

Results: Little formatting touch-ups

dc8c7b4

- Removing obsolete comment from tables.py - Converted name of current iron parameter from camelCase

tienne-B force-pushed the feature/1003-b branch from acb1635 to 387359e Compare August 10, 2019 03:52

tienne-B mentioned this pull request Aug 10, 2019

Create elimination ballot form for non-BP formats #1004

Closed

tienne-B force-pushed the feature/1003-b branch 3 times, most recently from 63f8970 to cf3d72c Compare August 10, 2019 10:12

czlee self-assigned this Aug 12, 2019

tienne-B added this to the M-Release milestone Aug 13, 2019

tienne-B force-pushed the feature/1003-b branch from cf3d72c to 5cb43ed Compare August 13, 2019 17:37

tienne-B mentioned this pull request Sep 2, 2019

Start developing Debate XML import/export #1076

Merged

tienne-B force-pushed the feature/1003-b branch 3 times, most recently from 4378c02 to f7f04c1 Compare September 5, 2019 03:04

tienne-B commented Sep 5, 2019

View reviewed changes

tabbycat/results/scoresheet.py Outdated Show resolved Hide resolved

tienne-B commented Sep 5, 2019

View reviewed changes

tienne-B force-pushed the feature/1003-b branch 4 times, most recently from d7f0b9e to 6194086 Compare September 5, 2019 15:02

tienne-B added 7 commits September 5, 2019 13:01

Results: Refactor forms for new DebateResult

504517f

This change makes the current forms usable with the new format of DebateResult, which will be refactored in another commit. It also marks the password label for translation and does some cleanup.

Add field for declared winners to ballot forms

403da13

This commit adds fields for adjudicators to precise the winning team on their ballots through a drop-down after their scores, using side names. Checks have also been added to results to check for empty lists when finding the winner.

tienne-B force-pushed the feature/1003-b branch from 6194086 to 194143f Compare September 5, 2019 17:02

tienne-B force-pushed the feature/1003-b branch 2 times, most recently from 60a743c to 88705fc Compare September 8, 2019 13:26

tienne-B requested a review from czlee May 6, 2020 16:22

czlee removed their assignment May 25, 2020

philipbelesky force-pushed the develop branch from 2000c80 to 9fa23c9 Compare May 31, 2020 01:34

czlee modified the milestones: Manx, N-Release Jun 13, 2020

Merge branch 'develop' into feature/1003-b

da06210

# Conflicts: # tabbycat/results/result.py # tabbycat/tournaments/models.py

tienne-B mentioned this pull request Aug 6, 2020

Allow dedicated reply speakers (w/o substantives) #1584

Closed

tienne-B added 4 commits September 14, 2020 17:00

Add results API to schema

dac1d4b

This commit adds the schema definition for the Results endpoints to the OpenAPI schema.

Merge branch 'develop' into feature/1003-b

b1e2a16

# Conflicts: # tabbycat/api/serializers.py

Make merge migration for results

937976f

czlee requested changes Oct 6, 2020

View reviewed changes

tienne-B added 2 commits October 9, 2020 00:26

Implement some fixes to result forms

fd51713

- Renamed "adj" message parameter to "adjudicator" - Added team sides to declared winners dropdown - Don't set declared winners with high-point wins - Specify adj in voting validation declared winner

Merge branch 'develop' into feature/1003-b

2eaebff

# Conflicts: # tabbycat/results/views.py

czlee approved these changes Oct 16, 2020

View reviewed changes

czlee merged commit 160fb3c into TabbycatDebate:develop Oct 16, 2020

czlee removed the awaiting maintainer Issues/PRs waiting on a maintainer label Oct 16, 2020

tienne-B deleted the feature/1003-b branch October 16, 2020 04:09

tienne-B added a commit to tienne-B/tabbycat that referenced this pull request Oct 16, 2020

Changelog: Add TabbycatDebate#1180

eaf680f

tienne-B added a commit that referenced this pull request Oct 16, 2020

Changelog: Add #1180

504d3bd

czlee mentioned this pull request Dec 27, 2020

populate_results tries to populate scores from trainee scoresheets #922

Closed

czlee mentioned this pull request Dec 27, 2020

ConsensusDebateResult causes N+1 queries in admin results page #1704

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor DebateResult and associated classes #1180

Refactor DebateResult and associated classes #1180

tienne-B commented Aug 10, 2019

tienne-B Sep 5, 2019

czlee Oct 5, 2020

czlee left a comment

czlee Oct 5, 2020

czlee commented Oct 6, 2020

czlee commented Oct 6, 2020

philipbelesky commented Oct 6, 2020

tienne-B commented Oct 9, 2020

czlee commented Oct 13, 2020

czlee left a comment

czlee Oct 16, 2020

tienne-B Oct 16, 2020

czlee Oct 21, 2020

czlee commented Dec 27, 2020

Refactor DebateResult and associated classes #1180

Refactor DebateResult and associated classes #1180

Conversation

tienne-B commented Aug 10, 2019

tienne-B Sep 5, 2019

Choose a reason for hiding this comment

czlee Oct 5, 2020

Choose a reason for hiding this comment

czlee left a comment

Choose a reason for hiding this comment

czlee Oct 5, 2020

Choose a reason for hiding this comment

czlee commented Oct 6, 2020

czlee commented Oct 6, 2020

philipbelesky commented Oct 6, 2020

tienne-B commented Oct 9, 2020

czlee commented Oct 13, 2020

czlee left a comment

Choose a reason for hiding this comment

czlee Oct 16, 2020

Choose a reason for hiding this comment

tienne-B Oct 16, 2020

Choose a reason for hiding this comment

czlee Oct 21, 2020

Choose a reason for hiding this comment

czlee commented Dec 27, 2020