diff --git a/frontend/src/components/Round/RoundEdit.vue b/frontend/src/components/Round/RoundEdit.vue index 467249e3..22395a36 100644 --- a/frontend/src/components/Round/RoundEdit.vue +++ b/frontend/src/components/Round/RoundEdit.vue @@ -114,7 +114,7 @@ const formData = ref({ deadline_date: props.round.deadline_date.split('T')[0], config: props.round.config, show_stats: props.round.show_stats, - jurors: props.round.jurors.map((juror) => juror.username) + jurors: props.round.jurors.filter((juror) => juror.is_active).map((juror) => juror.username) }) const showStatsOptions = ref([ @@ -169,6 +169,7 @@ const saveRound = () => { .editRound(round.id, round) .then(() => { emit('update:isRoundEditing', false) + window.location.reload() }) .catch(alertService.error) } diff --git a/montage/admin_endpoints.py b/montage/admin_endpoints.py index a8faabfc..0fb35bbe 100644 --- a/montage/admin_endpoints.py +++ b/montage/admin_endpoints.py @@ -62,6 +62,7 @@ def get_admin_routes(): POST('/admin/round//finalize', finalize_round), GET('/admin/round/', get_round), POST('/admin/round//edit', edit_round), + POST('/admin/round//remove_juror', remove_juror), POST('/admin/round//cancel', cancel_round), GET('/admin/round//preview_results', get_round_results_preview), @@ -525,6 +526,36 @@ def edit_round(user_dao, round_id, request_dict): return {'data': new_val_map} +def remove_juror(user_dao, round_id, request_dict): + """ + Summary: Remove a juror from a paused round, cancelling their open + tasks. Their completed votes are kept unless + discard_completed is set. For yesno/rating rounds the + quorum drops to new_quorum (or to the remaining jury size + when that is smaller than the current quorum). + + Request model: + username: + type: string + discard_completed: + type: boolean + new_quorum: + type: int64 + + Response model: RemoveJurorSummary + """ + coord_dao = CoordinatorDAO.from_round(user_dao, round_id) + username = request_dict.get('username') + if not username or not isinstance(username, str): + raise InvalidAction('expected a username') + discard_completed = bool(request_dict.get('discard_completed', False)) + new_quorum = request_dict.get('new_quorum') + summary = coord_dao.remove_juror(round_id, username, + discard_completed=discard_completed, + new_quorum=new_quorum) + return {'data': summary} + + def cancel_round(user_dao, round_id): coord_dao = CoordinatorDAO.from_round(user_dao, round_id) rnd = coord_dao.cancel_round(round_id) @@ -545,6 +576,7 @@ def get_round_results_preview(user_dao, round_id): if rnd.vote_method in ('yesno', 'rating'): data['ratings'] = coord_dao.get_round_average_rating_map(round_id) + data['damped_ratings'] = coord_dao.get_round_damped_rating_map(round_id) try: data['thresholds'] = get_threshold_map(data['ratings']) except: diff --git a/montage/rdb.py b/montage/rdb.py index f0894377..5d77835b 100644 --- a/montage/rdb.py +++ b/montage/rdb.py @@ -43,7 +43,7 @@ to_unicode, get_mw_userid, weighted_choice, - PermissionDenied, InvalidAction, NotImplementedResponse, + PermissionDenied, InvalidAction, DoesNotExist, get_env_name, load_default_series, @@ -445,6 +445,7 @@ def to_info_dict(self): 'status': self.status, 'config': self.config, 'show_stats': self.show_stats, + 'quorum': self.quorum, 'round_sources': []} return ret @@ -452,10 +453,9 @@ def to_details_dict(self): ret = self.to_info_dict() ret['is_closable'] = self.check_closability() ret['campaign'] = self.campaign.to_info_dict() - ret['quorum'] = self.quorum ret['total_round_entries'] = len(self.round_entries) ret['stats'] = self.get_count_map() - ret['juror_details'] = [rj.to_details_dict() for rj in self.round_jurors], + ret['juror_details'] = [rj.to_details_dict() for rj in self.round_jurors] return ret def confirm_active(self): @@ -1372,6 +1372,16 @@ def edit_round(self, round_id, round_dict): # if round_dict ...: # raise InvalidAction('unable to modify round attributes: %r' # % request_dict.keys()) + # quorum applies before jurors, so a single save can lower the + # quorum and shrink the jury together (the juror-edit guard + # checks the new roster against the already-updated quorum) + new_quorum = round_dict.get('quorum') + if new_quorum and new_quorum != rnd.quorum: + if rnd.status != PAUSED_STATUS: + raise InvalidAction('round must be paused to edit quorum') + else: + new_juror_stats = self.modify_quorum(round_id, new_quorum) + new_val_map['quorum'] = new_quorum new_juror_names = round_dict.get('new_jurors') cur_jurors = self.get_active_jurors(round_id) cur_juror_names = [u.username for u in cur_jurors] @@ -1383,13 +1393,6 @@ def edit_round(self, round_id, round_dict): raise InvalidAction('new_jurors must be a list of strings') new_juror_stats = self.modify_jurors(round_id, new_juror_names) new_val_map['new_jurors'] = new_juror_names - new_quorum = round_dict.get('quorum') - if new_quorum and new_quorum != rnd.quorum: - if rnd.status != PAUSED_STATUS: - raise InvalidAction('round must be paused to edit quorum') - else: - new_juror_stats = self.modify_quorum(round_id, new_quorum) - new_val_map['quorum'] = new_quorum show_stats = round_dict.get('show_stats') if show_stats is not None: rnd.show_stats = show_stats @@ -2040,6 +2043,7 @@ def get_rating_advancing_group(self, round_id, threshold=None): .options(joinedload('entry'))\ .filter_by(dq_user_id=None, round_id=round_id)\ .join(Vote)\ + .filter(Vote.status == COMPLETED_STATUS)\ .group_by(Vote.round_entry_id)\ .having(avg >= threshold)\ .all() @@ -2061,6 +2065,36 @@ def get_round_average_rating_map(self, round_id): return dict(rating_ctr) + def get_round_damped_rating_map(self, round_id): + """Same shape as get_round_average_rating_map, but each entry's + average is damped toward the round's overall mean C with prior + weight m = quorum (a Bayesian average): + + damped = (n * avg + m * C) / (n + m) + + Entries whose plain average rests on few completed votes get + pulled toward C, so coordinators can spot scores that are still + unstable (e.g. after a quorum decrease or juror removal). + """ + rnd = self.get_round(round_id) + results = self.query(func.avg(Vote.value), func.count(Vote.id))\ + .join(RoundEntry, RoundEntry.id == Vote.round_entry_id)\ + .filter(RoundEntry.round_id == round_id, + Vote.status == COMPLETED_STATUS)\ + .group_by(Vote.round_entry_id)\ + .all() + if not results: + return {} + + total_count = sum([count for _, count in results]) + overall_mean = sum([avg * count for avg, count in results]) / total_count + m = rnd.quorum or 1 + + damped_ctr = Counter([round((count * avg + m * overall_mean) + / (count + m), 3) + for avg, count in results]) + return dict(damped_ctr) + def get_round_ranking_list(self, round_id, notation=None): res = (self.query(Vote) .join(RoundEntry, RoundEntry.id == Vote.round_entry_id) @@ -2219,9 +2253,10 @@ def modify_jurors(self, round_id, new_jurors, force_balance=False): if rnd.vote_method == 'ranking': rnd.quorum = len(new_jurors) elif rnd.quorum > len(new_jurors): - raise InvalidAction('expected at least %s jurors to make quorum' - ' (%s) for round #%s' - % (len(new_jurors), rnd.quorum, rnd.id)) + raise InvalidAction('round #%s has quorum %s, which requires at' + ' least %s jurors; lower the quorum first (or' + ' in the same edit), or use the remove_juror' + ' endpoint' % (rnd.id, rnd.quorum, rnd.quorum)) new_juror_names = sorted([nj.username for nj in new_jurors]) old_jurors = self.get_active_jurors(rnd.id) old_juror_names = sorted([oj.username for oj in old_jurors]) @@ -2229,10 +2264,11 @@ def modify_jurors(self, round_id, new_jurors, force_balance=False): if new_juror_names == old_juror_names: raise InvalidAction('new jurors must differ from current jurors') - if len(new_jurors) == len(old_jurors) and not force_balance: - added_juror = list(set(new_jurors) - set(old_jurors))[0] - removed_juror = list(set(old_jurors) - set(new_jurors))[0] - res = swap_tasks(self.rdb_session, rnd, added_juror, removed_juror) + added = list(set(new_jurors) - set(old_jurors)) + removed = list(set(old_jurors) - set(new_jurors)) + if (len(added) == 1 and len(removed) == 1 + and len(new_jurors) == len(old_jurors) and not force_balance): + res = swap_tasks(self.rdb_session, rnd, added[0], removed[0]) else: res = reassign_tasks(self.rdb_session, rnd, new_jurors) @@ -2254,47 +2290,184 @@ def modify_jurors(self, round_id, new_jurors, force_balance=False): self.log_action('modify_jurors', round=rnd, message=msg) return res - def modify_quorum(self, round_id, new_quorum, strategy=None): - # This only supports increasing the quorum. Decreaseing the - # quorum would require handling some completed tasks (eg, - # whose vote do you discard? Randomly choose?) - + def remove_juror(self, round_id, username, + discard_completed=False, new_quorum=None): + """Remove a single juror from a paused round, cancelling their + not-yet-cast tasks. Their completed votes are kept unless + *discard_completed* is set (discarded votes flip to cancelled, + values retained on the rows for audit, and affected entries are + re-covered by the remaining jurors where possible). + + For yesno/rating rounds the quorum drops to *new_quorum* when + given, else to the remaining jury size when that is smaller + than the current quorum. + """ rnd = self.get_round(round_id) + if rnd.status != PAUSED_STATUS: + raise InvalidAction('round must be paused to remove a juror') + + removed_rj = None + for rj in rnd.round_jurors: + if rj.user.username == username and rj.is_active: + removed_rj = rj + break + if removed_rj is None: + raise InvalidAction('user %r is not an active juror of round #%s' + % (username, rnd.id)) + removed_user = removed_rj.user + + remaining = [rj.user for rj in rnd.round_jurors + if rj.is_active and rj.user.id != removed_user.id] + if not remaining: + raise InvalidAction('cannot remove the last juror') + session = self.rdb_session + now = datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) old_quorum = rnd.quorum - if not new_quorum: - raise InvalidAction('must specify new quorum') + # discard first, so the excess-task cancellation below sees the + # true per-entry coverage and does not cancel tasks it would + # then have to recreate + discarded_completed_count = 0 + if discard_completed: + completed_votes = (session.query(Vote) + .filter_by(status=COMPLETED_STATUS, + user=removed_user) + .join(RoundEntry) + .filter_by(round=rnd) + .all()) + # values stay on the rows for audit; scoring only reads + # completed votes, so cancelled rows drop out of results + for vote in completed_votes: + vote.status = CANCELLED_STATUS + vote.modified_date = now + discarded_completed_count = len(completed_votes) - if new_quorum <= old_quorum: - raise NotImplementedResponse('currently we do not support quorum ' - 'decreases. current quorum is %r, got %r' - % (old_quorum, new_quorum)) + if rnd.vote_method == 'ranking': + if new_quorum is not None: + raise InvalidAction('no quorum for a ranking round') + cancelled_active_count = (session.query(Vote) + .filter_by(status=ACTIVE_STATUS, + user=removed_user) + .join(RoundEntry) + .filter_by(round=rnd) + .count()) + res = self.modify_jurors(round_id, + [u.username for u in remaining]) + created_task_count, uncoverable_entry_count = 0, 0 + else: + if new_quorum is not None: + if not 1 <= new_quorum <= len(remaining): + raise InvalidAction('new quorum must be between 1 and' + ' the number of remaining jurors' + ' (%s), got %r' + % (len(remaining), new_quorum)) + target_quorum = new_quorum + else: + target_quorum = min(rnd.quorum, len(remaining)) - jurors = self.get_active_jurors(rnd.id) - session = self.rdb_session + active_votes = (session.query(Vote) + .filter_by(status=ACTIVE_STATUS, + user=removed_user) + .join(RoundEntry) + .filter_by(round=rnd) + .all()) + for vote in active_votes: + vote.status = CANCELLED_STATUS + vote.modified_date = now + cancelled_active_count = len(active_votes) + + removed_rj.is_active = 0 + + if target_quorum < rnd.quorum: + cancel_excess_rating_tasks(session, rnd, target_quorum) + rnd.quorum = target_quorum + + # re-cover entries the cancellations/discards pushed below + # quorum, where remaining jurors who never saw them exist; + # unavoidable shortfalls are counted, not fatal (the mean + # stays defined for any completed count) + created_votes, uncoverable_entry_count = \ + create_rating_tasks_for_coverage( + session, rnd, target_per_entry=rnd.quorum) + created_task_count = len(created_votes) + res = reassign_rating_tasks(session, rnd, remaining) + + msg = ('%s removed juror %s from round #%s (discard_completed=%r,' + ' quorum %r -> %r): cancelled %s open tasks, discarded %s' + ' completed votes, created %s replacement tasks' + % (self.user.username, username, rnd.id, discard_completed, + old_quorum, rnd.quorum, cancelled_active_count, + discarded_completed_count, created_task_count)) + self.log_action('remove_juror', round=rnd, message=msg) + + return {'removed_juror': username, + 'cancelled_active_count': cancelled_active_count, + 'discarded_completed_count': discarded_completed_count, + 'created_task_count': created_task_count, + 'uncoverable_entry_count': uncoverable_entry_count, + 'quorum': rnd.quorum, + 'task_count_mean': res['task_count_mean']} - rnd_stats = rnd.to_details_dict() - if not rnd_stats['stats']['total_tasks']: - create_initial_tasks(session, rnd) + def modify_quorum(self, round_id, new_quorum, strategy=None): + """Change the quorum of a yesno/rating round mid-round. - rnd.quorum = new_quorum + Increases create just enough new tasks to bring every entry up + to the new quorum. Decreases cancel excess not-yet-cast tasks; + completed votes are never discarded (quorum is a coverage + floor, so entries may keep more completed votes than quorum). + """ + rnd = self.get_round(round_id) + old_quorum = rnd.quorum if rnd.vote_method == 'ranking': raise InvalidAction('no quorum for a ranking round') - elif rnd.vote_method in ('yesno', 'rating'): - new_tpe = new_quorum - old_quorum - # I'm pretty sure this will fairly distribute tasks - created_tasks = create_initial_rating_tasks(session, rnd, - tasks_per_entry=new_tpe) + if rnd.vote_method not in ('yesno', 'rating'): + raise ValueError('invalid vote method: %r' % rnd.vote_method) + if not new_quorum or new_quorum < 1: + raise InvalidAction('must specify a positive quorum') + if new_quorum == old_quorum: + raise InvalidAction('new quorum matches current quorum (%r)' + % old_quorum) + + jurors = self.get_active_jurors(rnd.id) + if new_quorum > len(jurors): + raise InvalidAction('quorum cannot be greater than the number' + ' of jurors') + + session = self.rdb_session + + if not rnd._get_task_count(): + create_initial_tasks(session, rnd) + + created_task_count, cancelled_task_count = 0, 0 + if new_quorum > old_quorum: + created_votes, _ = create_rating_tasks_for_coverage( + session, rnd, target_per_entry=new_quorum) + created_task_count = len(created_votes) + rnd.quorum = new_quorum ret = reassign_rating_tasks(session, rnd, jurors, - strategy=strategy, reassign_all=True) + strategy=strategy, + reassign_all=True) else: - raise ValueError('invalid vote method: %r' % rnd.vote_method) + cancel_stats = cancel_excess_rating_tasks( + session, rnd, target_per_entry=new_quorum) + cancelled_task_count = cancel_stats['cancelled_task_count'] + rnd.quorum = new_quorum + # no reassign_all: jurors keep the tasks they hold, only + # over-full queues shed tasks (less juror churn) + ret = reassign_rating_tasks(session, rnd, jurors, + strategy=strategy) - msg = ('%s changed round #%s quorum (%r -> %r), reassigned %s tasks' - ' (average juror task queue is now at %s)' + ret['created_task_count'] = created_task_count + ret['cancelled_task_count'] = (cancelled_task_count + + ret.get('cancelled_task_count', 0)) + + msg = ('%s changed round #%s quorum (%r -> %r), created %s and' + ' cancelled %s tasks, reassigned %s (average juror task' + ' queue is now at %s)' % (self.user.username, rnd.id, old_quorum, new_quorum, + ret['created_task_count'], ret['cancelled_task_count'], ret['reassigned_task_count'], ret['task_count_mean'])) self.log_action('modify_quorum', round=rnd, message=msg) @@ -3189,12 +3362,13 @@ def create_initial_rating_tasks(rdb_session, rnd, tasks_per_entry=None): if not tasks_per_entry: tasks_per_entry = rnd.quorum - if tasks_per_entry > len(rnd.round_jurors): - raise InvalidAction('quorum cannot be greater than the number of jurors') - jurors = [rj.user for rj in rnd.round_jurors if rj.is_active] if not jurors: raise InvalidAction('expected round with active jurors') + + if tasks_per_entry > len(jurors): + raise InvalidAction('quorum cannot be greater than the number of jurors') + random.shuffle(jurors) shuffled_entries = _shuffled_untasked_entry_ids(rdb_session, rnd) @@ -3225,6 +3399,135 @@ def create_initial_rating_tasks(rdb_session, rnd, tasks_per_entry=None): return _insert_tasks(rdb_session, tasks) +def create_rating_tasks_for_coverage(session, rnd, target_per_entry): + """Bring every non-disqualified entry of *rnd* up to + *target_per_entry* total non-cancelled (active or completed) votes. + + Unlike create_initial_rating_tasks, this covers entries that + already have votes, so it works mid-round (quorum increases, + re-covering entries after a juror's completed votes are + discarded). + + Each new task goes to the eligible active juror with the fewest + open tasks in the round (ties broken by user id, for determinism). + A juror is eligible for an entry if they have no active or + completed vote on it; a cancelled vote does not disqualify. + + Returns ``(created_votes, shortfall_entry_count)``, the latter + counting entries with fewer eligible jurors than needed tasks + (they get as many as possible; the entry mean stays defined for + any completed count). + """ + jurors = [rj.user for rj in rnd.round_jurors if rj.is_active] + if not jurors: + raise InvalidAction('expected round with active jurors') + + round_entries = (session.query(RoundEntry) + .filter(RoundEntry.round_id == rnd.id, + RoundEntry.dq_user_id == None) + .all()) + + votes = (session.query(Vote) + .join(RoundEntry, RoundEntry.id == Vote.round_entry_id) + .filter(RoundEntry.round_id == rnd.id, + Vote.status.in_((ACTIVE_STATUS, + COMPLETED_STATUS))) + .all()) + + have_map = defaultdict(list) # round_entry_id -> [Vote, ...] + open_count_map = dict([(j.id, 0) for j in jurors]) + for vote in votes: + have_map[vote.round_entry_id].append(vote) + if vote.status == ACTIVE_STATUS and vote.user_id in open_count_map: + open_count_map[vote.user_id] += 1 + + juror_map = dict([(j.id, j) for j in jurors]) + + created_votes = [] + shortfall_entry_count = 0 + for round_entry in round_entries: + cur_votes = have_map[round_entry.id] + needed = target_per_entry - len(cur_votes) + if needed <= 0: + continue + seen_ids = set([v.user_id for v in cur_votes]) + elig_ids = [j_id for j_id in juror_map if j_id not in seen_ids] + if len(elig_ids) < needed: + shortfall_entry_count += 1 + for _ in range(min(needed, len(elig_ids))): + j_id = min(elig_ids, key=lambda j: (open_count_map[j], j)) + elig_ids.remove(j_id) + vote = Vote(user=juror_map[j_id], round_entry=round_entry, + status=ACTIVE_STATUS) + session.add(vote) + created_votes.append(vote) + open_count_map[j_id] += 1 + + return created_votes, shortfall_entry_count + + +def cancel_excess_rating_tasks(session, rnd, target_per_entry): + """Cancel not-yet-cast (active) votes in excess of + *target_per_entry* per entry. Completed votes are never touched: + an entry with more completed votes than the target simply keeps + them all (quorum is a coverage floor, not an exact count). + + Excess tasks are cancelled from the jurors with the most open + tasks in the round first (running counts; ties broken by vote id, + newest first), evening out juror queues while entry coverage stays + at or above the target by construction. + + Returns a dict with ``cancelled_task_count`` and + ``entries_below_target`` (informational: entries whose total + non-cancelled votes are under the target, only possible via prior + discards, never via this function). + """ + now = datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) + + round_entries = (session.query(RoundEntry) + .filter(RoundEntry.round_id == rnd.id, + RoundEntry.dq_user_id == None) + .all()) + + votes = (session.query(Vote) + .join(RoundEntry, RoundEntry.id == Vote.round_entry_id) + .filter(RoundEntry.round_id == rnd.id, + RoundEntry.dq_user_id == None, + Vote.status.in_((ACTIVE_STATUS, + COMPLETED_STATUS))) + .all()) + + entry_vote_map = defaultdict(list) + open_count_map = defaultdict(int) + for vote in votes: + entry_vote_map[vote.round_entry_id].append(vote) + if vote.status == ACTIVE_STATUS: + open_count_map[vote.user_id] += 1 + + cancelled_task_count = 0 + entries_below_target = 0 + for round_entry in round_entries: + re_votes = entry_vote_map[round_entry.id] + if len(re_votes) < target_per_entry: + entries_below_target += 1 + continue + completed_count = len([v for v in re_votes + if v.status == COMPLETED_STATUS]) + active = [v for v in re_votes if v.status == ACTIVE_STATUS] + keep_active = max(0, target_per_entry - completed_count) + for _ in range(len(active) - keep_active): + vote = max(active, + key=lambda v: (open_count_map[v.user_id], v.id)) + active.remove(vote) + vote.status = CANCELLED_STATUS + vote.modified_date = now + open_count_map[vote.user_id] -= 1 + cancelled_task_count += 1 + + return {'cancelled_task_count': cancelled_task_count, + 'entries_below_target': entries_below_target} + + def reassign_tasks(session, rnd, new_jurors, strategy=None): if rnd.vote_method in ('yesno', 'rating'): ret = reassign_rating_tasks(session, rnd, new_jurors, @@ -3273,7 +3576,7 @@ def reassign_ranking_tasks(session, rnd, new_jurors, strategy=None): old_juror_id_set = set([j.id for j in old_jurors]) new_juror_id_set = set([j.id for j in new_jurors]) if new_juror_id_set == old_juror_id_set: - return + return {'reassigned_task_count': 0, 'task_count_mean': -1} removed_jurors = [j for j in old_jurors if j.id not in new_juror_id_set] added_jurors = [j for j in new_jurors if j.id not in old_juror_id_set] @@ -3385,16 +3688,29 @@ def choose_eligible(eligible_users): return weighted_choice(wcp) + now = datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) + cancelled_votes = [] + while reassg_queue: vote = reassg_queue.pop() - vote.user = choose_eligible(elig_map[vote.round_entry]) - elig_map[vote.round_entry].remove(vote.user) + eligible = elig_map[vote.round_entry] + if not eligible: + # every juror in new_jurors has already completed this + # entry: the orphaned task is unfulfillable and the entry's + # coverage is already met, so cancel it + vote.status = CANCELLED_STATUS + vote.modified_date = now + cancelled_votes.append(vote) + continue + vote.user = choose_eligible(eligible) + eligible.remove(vote.user) target_work_map[vote.user].append(vote) vote_count_map = dict([(u, len(t)) for u, t in target_work_map.items()]) return {'incomplete_task_count': len(incomp_votes), - 'reassigned_task_count': len(reassg_votes), + 'reassigned_task_count': len(reassg_votes) - len(cancelled_votes), + 'cancelled_task_count': len(cancelled_votes), 'task_count_map': vote_count_map, 'task_count_mean': mean(list(vote_count_map.values()))} diff --git a/montage/tests/test_reassign.py b/montage/tests/test_reassign.py new file mode 100644 index 00000000..b3d379e6 --- /dev/null +++ b/montage/tests/test_reassign.py @@ -0,0 +1,71 @@ +# -*- coding: utf-8 -*- +"""DAO-free unit tests for the module-level task (re)assignment helpers +in montage.rdb, against an in-memory sqlite database. +""" + +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +from montage.rdb import (Base, + User, + Round, + Entry, + RoundEntry, + Vote, + reassign_rating_tasks, + ACTIVE_STATUS, + PAUSED_STATUS, + COMPLETED_STATUS, + CANCELLED_STATUS) + + +def make_session(): + engine = create_engine('sqlite://') + Base.metadata.create_all(engine) + return sessionmaker(bind=engine)() + + +def test_reassign_cancels_unfulfillable_task(): + """Regression: reassigning an active vote on an entry that every + remaining juror has already completed used to crash with a + ValueError from weighted_choice. The orphaned task is unfulfillable + (coverage is already met), so it must be cancelled instead. + """ + session = make_session() + + juror_a = User(id=1, username='JurorA') + juror_b = User(id=2, username='JurorB') + juror_c = User(id=3, username='JurorC') + + rnd = Round(name='test rating round', + vote_method='rating', + quorum=2, + status=PAUSED_STATUS, + jurors=[juror_a, juror_b, juror_c]) + entry = Entry(name='test entry.jpg') + round_entry = RoundEntry(entry=entry, round=rnd) + + vote_a = Vote(user=juror_a, round_entry=round_entry, + status=COMPLETED_STATUS, value=0.8) + vote_b = Vote(user=juror_b, round_entry=round_entry, + status=COMPLETED_STATUS, value=0.6) + vote_c = Vote(user=juror_c, round_entry=round_entry, + status=ACTIVE_STATUS) + + session.add_all([rnd, round_entry, vote_a, vote_b, vote_c]) + session.flush() + + # juror C leaves; A and B have both already rated the entry, so C's + # open task has no eligible juror and must be cancelled, not crash + res = reassign_rating_tasks(session, rnd, [juror_a, juror_b]) + session.flush() + + assert res['cancelled_task_count'] == 1 + assert vote_c.status == CANCELLED_STATUS + assert vote_c.modified_date is not None + + # completed votes are untouched and no new tasks appeared + assert vote_a.status == COMPLETED_STATUS + assert vote_b.status == COMPLETED_STATUS + active_votes = session.query(Vote).filter_by(status=ACTIVE_STATUS).all() + assert active_votes == [] diff --git a/montage/tests/test_web_basic.py b/montage/tests/test_web_basic.py index eb7bd913..d8b185f0 100644 --- a/montage/tests/test_web_basic.py +++ b/montage/tests/test_web_basic.py @@ -376,17 +376,130 @@ def test_home_client(base_client, api_client, mock_external_apis): u'Jimbo Wales']}, as_user='LilyOfTheWest') + resp = fetch('coordinator: get round stats before quorum raise', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + stats = resp['data']['stats'] + entry_count = (resp['data']['total_entries'] + - stats['total_disqualified_entries']) + total_tasks_before = stats['total_tasks'] + resp = fetch('coordinator: raise quorum value', '/admin/round/%s/edit' % round_id, {'quorum': 4}, as_user='LilyOfTheWest') - resp = fetch('coordinator: try to reduce quorum (not supported)', + # regression (bug: quorum raise created zero tasks mid-round, since + # create_initial_rating_tasks only covers entries with no votes at all) + resp = fetch('coordinator: get round stats after quorum raise', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + stats = resp['data']['stats'] + assert stats['total_tasks'] == total_tasks_before + entry_count + assert resp['data']['quorum'] == 4 + + # combined-edit rollback: raising quorum beyond the shrunken juror + # list must 400 and leave quorum, jurors, and tasks untouched + resp = fetch('coordinator: conflicting quorum raise and juror shrink', '/admin/round/%s/edit' % round_id, - {'quorum': 1}, + {'quorum': 5, + 'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Jean-Frédéric']}, as_user='LilyOfTheWest', error_code=400) + resp = fetch('coordinator: confirm rollback of failed combined edit', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + assert resp['data']['quorum'] == 4 + assert len([j for j in resp['data']['jurors'] if j['is_active']]) == 5 + assert resp['data']['stats']['total_tasks'] == total_tasks_before + entry_count + + # combined lower-quorum + juror shrink in a single save: the new jury + # (4) is smaller than the old quorum (4 would still fit, so drop to a + # 4-juror roster with quorum 2); quorum must apply before the juror edit + resp = fetch('coordinator: lower quorum and shrink jury in one save', + '/admin/round/%s/edit' % round_id, + {'quorum': 2, + 'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Jean-Frédéric', + u'Jimbo Wales']}, + as_user='LilyOfTheWest') + + resp = fetch('coordinator: get round stats after combined edit', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + assert resp['data']['quorum'] == 2 + jd_map = {jd['username']: jd for jd in resp['data']['jurors']} + assert not jd_map['Effeietsanders']['is_active'] + assert jd_map['Effeietsanders']['stats']['total_open_tasks'] == 0 + stats = resp['data']['stats'] + assert stats['total_open_tasks'] == 2 * entry_count + assert stats['total_tasks'] == 2 * entry_count # no completed votes yet + + resp = fetch('coordinator: restore the full juror roster', + '/admin/round/%s/edit' % round_id, + data={'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Effeietsanders', + u'Jean-Frédéric', + u'Jimbo Wales']}, + as_user='LilyOfTheWest') + + # quorum-only decrease (used to be pinned as an unsupported 400) + resp = fetch('coordinator: reduce quorum by itself', + '/admin/round/%s/edit' % round_id, + {'quorum': 1}, + as_user='LilyOfTheWest') + + resp = fetch('coordinator: get round stats after quorum-only decrease', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + assert resp['data']['quorum'] == 1 + assert resp['data']['stats']['total_open_tasks'] == entry_count + + # regression (bug: a same-size edit replacing two or more jurors + # swapped only one arbitrary pair, stranding the other removed + # juror's open tasks and giving the other new juror none) + resp = fetch('coordinator: swap out two jurors at once', + '/admin/round/%s/edit' % round_id, + data={'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Jean-Frédéric', + u'Zack Aeon', + u'Zora Bellal']}, + as_user='LilyOfTheWest') + + resp = fetch('coordinator: check both jurors were swapped', + '/admin/round/%s' % round_id, + as_user='LilyOfTheWest') + jd_map = {jd['username']: jd for jd in resp['data']['jurors']} + for username in ('Effeietsanders', 'Jimbo Wales'): + assert not jd_map[username]['is_active'] + assert jd_map[username]['stats']['total_open_tasks'] == 0 + for username in ('Zack Aeon', 'Zora Bellal'): + assert jd_map[username]['is_active'] + assert jd_map[username]['stats']['total_open_tasks'] > 0 + + resp = fetch('coordinator: swap the original two jurors back in', + '/admin/round/%s/edit' % round_id, + data={'new_jurors': [u'Slaporte', + u'MahmoudHashemi', + u'Effeietsanders', + u'Jean-Frédéric', + u'Jimbo Wales']}, + as_user='LilyOfTheWest') + + # restore quorum 4 so every juror has a full queue for the juror + # endpoints below; at quorum 1 the reassignments can leave Jimbo + # Wales with fewer than the 3 open tasks the skip checks need + resp = fetch('coordinator: restore quorum before reactivating', + '/admin/round/%s/edit' % round_id, + {'quorum': 4}, + as_user='LilyOfTheWest') + resp = fetch('coordinator: reactivate our round', '/admin/round/%s/activate' % round_id, {'post': True}, @@ -592,6 +705,86 @@ def test_home_client(base_client, api_client, mock_external_apis): '/admin/round/%s/activate' % rnd_2_id, {'post': True}, as_user='LilyOfTheWest') + # remove_juror requires a paused round + resp = fetch('coordinator: remove_juror fails on an active round', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Effeietsanders'}, + as_user='LilyOfTheWest', + error_code=400) + + # give Jean-Frédéric some completed votes to exercise the + # keep-vs-discard behaviors below + resp = fetch('juror: JF fetches tasks before leaving', + '/juror/round/%s/tasks?count=5' % rnd_2_id, + as_user=u'Jean-Frédéric') + jf_ratings = [{'vote_id': t['id'], 'value': 0.75} + for t in resp['data']['tasks']] + assert jf_ratings + resp = fetch('juror: JF submits some ratings before leaving', + '/juror/round/%s/tasks/submit' % rnd_2_id, + {'ratings': jf_ratings}, + as_user=u'Jean-Frédéric') + + resp = fetch('coordinator: pause round 2 to remove jurors', + '/admin/round/%s/pause' % rnd_2_id, + {'post': True}, as_user='LilyOfTheWest') + + resp = fetch('coordinator: remove_juror rejects a non-juror', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Not A Juror'}, + as_user='LilyOfTheWest', + error_code=400) + + # remove a juror, keeping their completed votes (the default); the + # jury drops 4 -> 3, which still meets the quorum of 3 + resp = fetch('coordinator: remove a juror, keeping completed votes', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Effeietsanders'}, + as_user='LilyOfTheWest') + summary = resp['data'] + assert summary['removed_juror'] == u'Effeietsanders' + assert summary['cancelled_active_count'] > 0 + assert summary['discarded_completed_count'] == 0 + # every entry they were due to see is re-covered by the one + # remaining juror who has not seen it yet + assert summary['created_task_count'] == summary['cancelled_active_count'] + assert summary['uncoverable_entry_count'] == 0 + assert summary['quorum'] == 3 + + # remove another juror, discarding their completed votes; the jury + # drops 3 -> 2, so the quorum implicitly lowers to 2 + resp = fetch('coordinator: remove a juror, discarding their votes', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Jean-Frédéric', 'discard_completed': True}, + as_user='LilyOfTheWest') + summary = resp['data'] + assert summary['removed_juror'] == u'Jean-Frédéric' + assert summary['discarded_completed_count'] == len(jf_ratings) + # with 3 jurors and quorum 3, every entry had one vote per juror; + # dropping to quorum 2 leaves the remaining pair covering everything + assert summary['created_task_count'] == 0 + assert summary['uncoverable_entry_count'] == 0 + assert summary['quorum'] == 2 + + resp = fetch('coordinator: check round 2 state after removals', + '/admin/round/%s' % rnd_2_id, + as_user='LilyOfTheWest') + assert resp['data']['quorum'] == 2 + jd_map = {jd['username']: jd for jd in resp['data']['jurors']} + for username in (u'Effeietsanders', u'Jean-Frédéric'): + assert not jd_map[username]['is_active'] + assert jd_map[username]['stats']['total_open_tasks'] == 0 + + resp = fetch('coordinator: remove_juror rejects an oversized new_quorum', + '/admin/round/%s/remove_juror' % rnd_2_id, + {'username': u'Slaporte', 'new_quorum': 5}, + as_user='LilyOfTheWest', + error_code=400) + + resp = fetch('coordinator: reactivate round 2 after removals', + '/admin/round/%s/activate' % rnd_2_id, + {'post': True}, as_user='LilyOfTheWest') + submit_ratings(api_client, rnd_2_id) resp = fetch('juror: get votes stats for rating round', @@ -608,6 +801,14 @@ def test_home_client(base_client, api_client, mock_external_apis): '/admin/round/%s/preview_results' % rnd_2_id, as_user='LilyOfTheWest') + # the damped (Bayesian) mean is shown alongside the plain mean for + # rating rounds; every damped value sits between the plain extremes + assert resp['data']['damped_ratings'] + plain, damped = resp['data']['ratings'], resp['data']['damped_ratings'] + assert sum(damped.values()) == sum(plain.values()) + lo, hi = min(map(float, plain)), max(map(float, plain)) + assert all(lo <= float(v) <= hi for v in damped) + thresh_map = resp['data']['thresholds'] # TODO cur_thresh = [t for t, c in sorted(thresh_map.items()) if 0 < c <= 20][-1] @@ -712,6 +913,8 @@ def test_home_client(base_client, api_client, mock_external_apis): resp = fetch('coordinator: preview round 3 results', '/admin/round/%s/preview_results' % rnd_3_id, as_user='LilyOfTheWest') + # ranking rounds have no quorum, so no damped ratings either + assert 'damped_ratings' not in resp['data'] resp = fetch('coordinator: read round 3 reviews', '/admin/round/%s/reviews' % rnd_3_id, @@ -863,6 +1066,22 @@ def test_multiple_jurors(api_client, mock_external_apis): rnd_data, as_user='LilyOfTheWest') + round_id = resp['data']['id'] + + # a freshly-created round is paused, so jurors can be removed; + # removing the second-to-last juror works, the last is refused + resp = fetch('coordinator: remove a juror from a fresh round', + '/admin/round/%s/remove_juror' % round_id, + {'username': u'Haylad'}, + as_user='LilyOfTheWest') + assert resp['data']['removed_juror'] == u'Haylad' + + resp = fetch('coordinator: removing the last juror fails', + '/admin/round/%s/remove_juror' % round_id, + {'username': u'Slaporte'}, + as_user='LilyOfTheWest', + error_code=400) + def test_get_files_info_by_name(api_client): """GET /utils/file returns file_infos with file_id populated.""" diff --git a/tools/admin.py b/tools/admin.py index 42dfb8a0..5a99da0a 100644 --- a/tools/admin.py +++ b/tools/admin.py @@ -96,6 +96,7 @@ def main(): rnd_cmd.add(pause_round, name='pause') rnd_cmd.add(advance_round, name='advance') rnd_cmd.add(edit_round_quorum, name='edit-quorum') + rnd_cmd.add(remove_juror, name='remove-juror') rnd_cmd.add(check_round_dupes, name='check-dupes') rnd_cmd.add(apply_round_ratings, name='apply-ratings') rnd_cmd.add(retask_duplicate_ratings, name='retask-dupes') @@ -476,26 +477,69 @@ def import_gist(user_dao, round_id, url): print('++ added entries to round %s: %r' % (round_id, stats)) -def edit_round_quorum(maint_dao, round_id): +def edit_round_quorum(user_dao, round_id): "change the quorum of a given round, assigning and reassigning tasks as need be" - rnd = maint_dao.get_round(round_id) + coord_dao = CoordinatorDAO.from_round(user_dao, round_id) + rnd = coord_dao.get_round(round_id) old_quorum = rnd.quorum if rnd.status != 'paused': print('-- round must be paused to edit quorum, aborting') return - print(('!! new quorum cannot be lower than current quorum: %s' % old_quorum)) - print(('!! new quorum cannot be higher than the number of jurors: %s' % len(rnd.jurors))) + active_juror_count = len([rj for rj in rnd.round_jurors if rj.is_active]) + print(('-- round %s (%s) currently has quorum %s and %s active jurors' + % (rnd.id, rnd.name, old_quorum, active_juror_count))) + print('-- decreases cancel excess un-cast tasks; completed votes are always kept') + print('!! new quorum cannot be higher than the number of active jurors') new_quorum = int(input('?? New quorum: ')) - new_juror_stats = maint_dao.modify_quorum(rnd, new_quorum) + stats = coord_dao.modify_quorum(rnd.id, new_quorum) - maint_dao.rdb_session.commit() + coord_dao.rdb_session.commit() print(('++ changed quorum in round %s (%s) from %s (old quorum) to %s (new quorum)' % (rnd.id, rnd.name, old_quorum, new_quorum))) - print(('++ reassigned %s tasks, with mean load of %s tasks per juror' - % (new_juror_stats['reassigned_task_count'], new_juror_stats['task_count_mean']))) + print(('++ created %s and cancelled %s tasks, reassigned %s, with mean load' + ' of %s tasks per juror' + % (stats['created_task_count'], stats['cancelled_task_count'], + stats['reassigned_task_count'], stats['task_count_mean']))) + + return stats + - return new_juror_stats +def remove_juror(user_dao, round_id): + "remove a juror from a paused round, cancelling their open tasks" + coord_dao = CoordinatorDAO.from_round(user_dao, round_id) + rnd = coord_dao.get_round(round_id) + if rnd.status != 'paused': + print('-- round must be paused to remove a juror, aborting') + return + active_jurors = sorted([rj.user.username for rj in rnd.round_jurors + if rj.is_active]) + print(('-- round %s (%s) has quorum %s and active jurors: %s' + % (rnd.id, rnd.name, rnd.quorum, ', '.join(active_jurors)))) + username = input('?? Username to remove: ') + if not username: + print('-- username required, aborting') + return + discard = input('?? Discard their completed votes as well? [y/N]: ') + discard_completed = discard.strip().lower() in ('y', 'yes') + summary = coord_dao.remove_juror(rnd.id, username, + discard_completed=discard_completed) + + coord_dao.rdb_session.commit() + + print(('++ removed juror %s from round %s (%s)' + % (summary['removed_juror'], rnd.id, rnd.name))) + print(('++ cancelled %s open tasks, discarded %s completed votes,' + ' created %s replacement tasks (%s entries uncoverable),' + ' quorum now %s, mean juror queue %s' + % (summary['cancelled_active_count'], + summary['discarded_completed_count'], + summary['created_task_count'], + summary['uncoverable_entry_count'], + summary['quorum'], + summary['task_count_mean']))) + + return summary def advance_round(user_dao, round_id, debug):