✨ Voting Portal: Acclaimed Contests - #3077
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
| area_id: String, | ||
| } | ||
|
|
||
| fn validate_manual_tally_sheet_contest( |
There was a problem hiding this comment.
Why a dedicated helper for a bool check? I think this should be inline instead. If it is ought to make more checks, take the whole contest and apply those too?
| /// | ||
| /// Acclaimed contests are displayed to the voter but never encoded, so | ||
| /// every codec, validation and consistency path must go through this | ||
| /// instead of reading `contests` directly. The voting portal keeps a |
There was a problem hiding this comment.
To uphold this requirement, we need to make contests field private (preferably real private, not pub(crate), or at least very narrowly scoped). It comes with some small structural update needs, but per my limited checks it should be rather minimal, aside from test fixture creations. To cover the required cases, we can export helpers, including test-only (feature gated extra impl or mods) tooling, as we do in beyond.
While I think making the whole struct more conservative would be better I'm aware that it'd blow way beyond this PR's scope. Since this introduces a new invariant for data consistency, I think it is worth to consider.
If we prefer to go this way, we'd need to do the same for the js types as well, provide accessors for "votable" or "full" contest list.
If you think this is not the correct time/place to introduce this change, I'd agree for sure, but would ask to add a doc on the contests field, so that anyone reaching to it can be aware of the semantic difference. Maybe something like:
/// All the contest available in this style.
/// It may include those are display-only (e.g., skipped by checks, not encoded) too.
/// See [Self::votable_contests()] for details.
pub contests: Vec<Contest>,There was a problem hiding this comment.
let's do this in another ticket :)
| {translate(question, "name", i18n.language) || ""} | ||
| </Typography> | ||
| {displayedAcclamationDescription ? ( | ||
| <Typography variant="body2" className="contest-acclamation"> |
There was a problem hiding this comment.
I recommend using info style Alert instead (for all acclamation messages), I think it'd be more readable and nicer as UI.
| Ok(()) | ||
| } | ||
|
|
||
| fn sorted_ids(ids: &HashSet<&str>) -> Vec<String> { |
There was a problem hiding this comment.
| fn sorted_ids(ids: &HashSet<&str>) -> Vec<String> { | |
| fn sorted_ids<'a>(ids: &HashSet<&'a str>) -> Vec<&'a str> { | |
| let mut ids: Vec<_> = ids.iter().copied().collect(); | |
| ids.sort_unstable(); | |
| ids | |
| } |
There was a problem hiding this comment.
I think, we definitely need a guard on the endpoint publishing the changes to prevent acclamation change. An administrative/config issue shouldn't allow corrupting data or causing incorrectly decoded votes, but result in error or bad ux at most, I think.
There was a problem hiding this comment.
I'll add it, as even though it's not the focus of the PR, it's long overdue
| /// | ||
| /// Blank/invalid markers, withdrawn candidates, and empty write-in slots | ||
| /// are ballot configuration artefacts rather than elected candidates. | ||
| pub fn is_eligible_acclaimed_candidate(&self) -> bool { |
There was a problem hiding this comment.
I think this naming is quite confusing, maybe something like is_acclamation_eligible read better, took me a while to figure this out for real. If you agree, all the call sites and relevant field/functions/variables should be updated for the new naming.
2 participants
Parent issue: https://github.com/sequentech/meta/issues/12900