Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat: replace active quantifier #503

Merged
merged 32 commits into from
Jun 29, 2022
Merged

Feat: replace active quantifier #503

merged 32 commits into from
Jun 29, 2022

Conversation

mattyg
Copy link
Collaborator

@mattyg mattyg commented Jun 26, 2022 •
edited
Loading

Resolves #432

Overview

  • refactor button components MarkDismissedButton, MarkDuplicateButton to use a generic IconButton
  • refactor UserPopover to no longer use headlessui's Popover (as it only supports click-based popover and wasn't being utilized)
  • Resolve Allow admins to replace quantifiers #432
  • Remove cli command replace-quantifier

Note that we are currently including all quantifiers in the list of possible replacements, not just those currently unassigned.

mattyg added 18 commits June 22, 2022 14:03
@mattyg
fix(api): remove check to verify that *all* quantifiers were assigned…
9332450 
… to a bucket of praise in 'prepareAssignmentsByAllQuantifiers()' - as this is impossible if there are more quantifiers in pool than praise receivers in period
@mattyg
chore(api): remove unnecessary logging
fa1d935
@mattyg
fix(api): replace setting PRAISE_QUANTIIFIERS_ASSIGN_ALL with PRAISE_…
14709b0 
…QUANTIFIERS_ASSIGN_EVENLY to clarify that the algorithm cannot assign *all* quantifiers in some conditions (such as if there are more quantifiers than available receivers)
@mattyg
chore: changelog
594ae3f
@mattyg
chore: changelog re-wording
74c2814
@mattyg
chore(frontend): delete unused component
af61fab
@mattyg
chore(frontend): remove unncessary function call wrappers
604aa63
@mattyg
refactor(frontend): delete MarkDuplicateButton, MarkDismissedButton -…
8a7f06e 
…- replace with generic IconButton
@mattyg
feat(api): period controller to replace a quantifier
a02d54b
@mattyg
feat(frontend): UX to replace an active quantifier with another that …
a60dc4f 
…is unassigned
@mattyg
chore: remove no longer needed cli command 'replace-quantifier'
f95da2a
@mattyg
fix(frontend): typo on tailwind class name
9551bc9
@mattyg
style(frontend): make SelectUserRadioGroup expand height up to max size
090110e
@mattyg
refactor(frontend): remove headlessui Popover from UserPopover, as it…
269be5d 
… was not actually being used (it only supports click-based popovers)
@mattyg
style(frontend): support dark mode in SelectUserRadioGroup
0e26fb7
@mattyg
fix(api): ensure that quantification scores are completely reset when…
7597797 
… quantifier is replaced
@mattyg
refactor(api): use getPeriodDateRangeQuery to reduce duplicate logic
eba1fa2
@mattyg
fix(frontend): display Notice when no unassigned quantifiers are avai…
b963d21 
…lable as replcements
@mattyg
Copy link
Collaborator Author

mattyg commented Jun 27, 2022

Currently, only unassigned quantifiers appear in the list of possible replacements. Should we instead include all quantifiers in the list of possible replacements?

@kristoferlund

mattyg added 2 commits June 27, 2022 11:56
@mattyg
chore: changelog
8d86635
@mattyg
fix: after replacing quantifier, update both period and affected prai…
9ab0fac 
…ses in frontend store, to ensure praise quantification data is fresh
@mattyg mattyg changed the title WIP Feat: replace active quantifier Feat: replace active quantifier Jun 27, 2022
@mattyg mattyg requested a review from kristoferlund June 27, 2022 16:39
@mattyg mattyg marked this pull request as ready for review June 27, 2022 17:07
@mattyg mattyg changed the title Feat: replace active quantifier WIP Feat: replace active quantifier Jun 27, 2022
@mattyg
Copy link
Collaborator Author

mattyg commented Jun 27, 2022

Currently, only unassigned quantifiers appear in the list of possible replacements. Should we instead include all quantifiers in the list of possible replacements?

@kristoferlund

We decided to include all quantifiers for now.

@mattyg mattyg changed the title WIP Feat: replace active quantifier Feat: replace active quantifier Jun 27, 2022
@mattyg mattyg removed the request for review from kristoferlund June 27, 2022 20:21
@mattyg mattyg requested a review from kristoferlund June 27, 2022 20:21
kristoferlund
kristoferlund requested changes Jun 27, 2022
View reviewed changes
Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

One thing needs fixing though. Allowing the choice of any quantifier can cause a situation like this:

image

Perhaps we need to check if the new quantifier already have been assigned any of the receivers of the quantifier being replaced?

kristoferlund and others added 9 commits June 28, 2022 09:36
@kristoferlund
Styling adjustments
d229cdc
@kristoferlund
Merge branch 'main' into fix/allow_even_quantifier_assignment_when_to…
e3c7fe4 
…o_many_quantifiers
@mattyg
test(api): fix settings and periodsettings tests - use seeded setting…
f5fe398 
…s rather than expecting 'real' settings from migrations to exist
@mattyg
Merge branch 'fix/allow_even_quantifier_assignment_when_too_many_quan…
076ba41 
…tifiers' into feat/replace_active_quantifier
@mattyg
fix(api): prevent replacing quantifier with another that is already a…
a6b57f6 
…ssigned to some of the original quantifier's praise
@mattyg
fix(frontend): clear selected user after submit in ReplaceQuantifierD…
63e45e4 
…ialog
@mattyg
feat(frontend): exclude the original quantifier from the list of poss…
75c975b 
…ible replacements in ReplaceQuantifierDialog
@mattyg
fix(frontend): update replace quantifier success message to indicate …
dd7601f 
…the their scores are reset
@mattyg
test(api): tests for replaceQuantifiers controller
830d0cb
@mattyg
Copy link
Collaborator Author

mattyg commented Jun 28, 2022

Perhaps we need to check if the new quantifier already have been assigned any of the receivers of the quantifier being replaced?

good catch! Fixed.

Also made a few other changes:

  • Exclude the original quantifier from the list of possible replacements
  • Clear the selected user from the SelectUserRadioGroup after clicking "Replace" button
  • Update success message to clarify that the scores are reset
  • added tests for replaceQuantifier controller

@mattyg mattyg requested a review from kristoferlund June 28, 2022 19:22
kristoferlund
kristoferlund approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fixes! Merging.

packages/frontend/src/components/IconButton.tsx
onClick();
}

const MarkDuplicateButton = ({
const IconButton = ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@kristoferlund kristoferlund merged commit 40c389e into main Jun 29, 2022
@kristoferlund kristoferlund deleted the feat/replace_active_quantifier branch June 29, 2022 07:11
@kristoferlund kristoferlund mentioned this pull request Jul 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristoferlund kristoferlund kristoferlund approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow admins to replace quantifiers
2 participants
@mattyg @kristoferlund