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.

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

Fix/evenly assign quantifiers prevent overlap #573

Merged
merged 7 commits into from
Sep 9, 2022

Conversation

mattyg
Copy link
Collaborator

@mattyg mattyg commented Sep 8, 2022

Resolves #535

  • Verify that no quantifier has been assigned to same receiver multiple times, before generating assignments
  • Refactor prepareAssignmentsEvenly to prevent a case where a quantifier could be assigned to the same receiver twice
  • Improve doc comments explaining prepareAssignmentsEvenly

Post-Mortem
Previously in prepareAssignmentsEvenly we:

  1. Generated a list of unique groups of receivers, duplicating each receiver for redundant assignments
  2. Attempted to evenly distribute the groups of receivers among all quantifiers, based on the total number of praise in the group of receivers (i.e. a quantifier could be assigned to multiple groups of receivers).

This allowed a case where a quantifier could be assigned a duplicate receivers, because multiple groups of unique receivers could potential contain a duplicate between them.

With this refactor, we swapped the order of these steps. Now we:

  1. Attempt to evenly distribute receivers among all quantifiers, based on the total amount of praise each receiver has (i.e. a quantifier could be assigned multiple receivers).
  2. Duplicating the assignments, rotating the order, for each redundant quantification.

@mattyg mattyg marked this pull request as ready for review September 8, 2022 20:04
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.4% 2.4% Duplication

@kristoferlund
Copy link
Member

Great work @mattyg!! I have tested it as best I can with some different period/quantifier settings. Assignments look flawless!

I added a couple of small frontend additions to better handle error messages when quantifiers cannot be assigned.

Merging!

@kristoferlund kristoferlund merged commit 926cd13 into main Sep 9, 2022
@kristoferlund kristoferlund deleted the fix/evenly-assign-quantifiers-prevent-overlap branch September 9, 2022 11:37
@kristoferlund kristoferlund mentioned this pull request Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quantify screen lists wrong number of praise
2 participants