Skip to content

feat: allow comparing list sizes against a number#1530

Merged
benscobie merged 13 commits into
Maintainerr:mainfrom
bitnimble:bitnimble/feat-list-count
Mar 31, 2025
Merged

feat: allow comparing list sizes against a number#1530
benscobie merged 13 commits into
Maintainerr:mainfrom
bitnimble:bitnimble/feat-list-count

Conversation

@bitnimble

@bitnimble bitnimble commented Jan 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR:

  • Creates new rule types, NUMBER_LIST and TEXT_LIST (I chose "list" to align with the term already used in the UI/copy)
  • Adds new rule actions for those rule types - COUNT_EQUALS, COUNT_NOT_EQUALS, COUNT_BIGGER, COUNT_SMALLER
  • Moves the existing rule possibilities/actions that were only applicable to lists from NUMBER and TEXT to the new rule types
  • Updates all existing rules that are lists to use the new RuleType
  • Adds tests, updates the frontend, etc to accommodate for the new rule type
  • Also does a small amount of cleanup of types, which was so I could make sure that I wasn't accidentally missing any cases when updating doRuleAction to support the new potential value types. I want to do a lot more cleanup here, but I'll do that in its own refactoring PR instead.

I think this requires no migration, since all rule actions that could have been applicable to list values previously are still valid on the new rule types (it is a superset). But I'm not 100% sure if I'm looking at the right place for how the rule actions are serialised and persisted.

image

image

bitnimble
bitnimble commented Jan 24, 2025
View reviewed changes
Comment thread server/src/modules/rules/getter/getter.service.ts
bitnimble
bitnimble commented Jan 24, 2025
View reviewed changes
Comment thread server/src/modules/rules/helpers/rule.comparator.service.ts
Comment thread server/src/modules/rules/helpers/rule.comparator.service.ts
Comment thread ui/src/components/Rules/Rule/RuleCreator/RuleInput/index.tsx
Comment thread ui/src/components/Rules/Rule/RuleCreator/RuleInput/index.tsx
Comment thread ui/src/contexts/constants-context.tsx
benscobie
benscobie reviewed Jan 24, 2025
View reviewed changes
Comment thread server/src/modules/rules/constants/rules.constants.ts
benscobie
benscobie reviewed Jan 24, 2025
View reviewed changes
Comment thread server/src/modules/rules/helpers/rule.comparator.service.ts Outdated
benscobie
benscobie reviewed Jan 25, 2025
View reviewed changes
Comment thread server/src/modules/rules/constants/rules.constants.ts Outdated
benscobie
benscobie reviewed Jan 25, 2025
View reviewed changes
Comment thread server/src/modules/rules/constants/rules.constants.ts
benscobie
benscobie reviewed Jan 26, 2025
View reviewed changes
Comment thread server/src/modules/rules/constants/rules.constants.ts Outdated
@bitnimble

bitnimble commented Feb 14, 2025

Copy link
Copy Markdown
Contributor Author

There are a few places in the FE that are technically a bug because we now have a situation where the rule type !== value type. e.g. a rule that has a text_list type has a custom value type of number (since the operation is a "count").
This doesn't end up actually resulting in anything bad happening though, since one of the React effects causes it to re-parse the firstVal type, resetting the rule type back to the correct value...

I'd like to fix that in the next PR since it unfortunately requires a big of rethinking around the RuleType type and that might end up being a change that touches quite a few places, and I don't believe it causes any visible issue at the moment.

@bitnimble bitnimble requested a review from benscobie February 14, 2025 06:27
@bitnimble

bitnimble commented Feb 14, 2025

Copy link
Copy Markdown
Contributor Author

If we want to be sure that nothing breaks though, I can always start work on that change first, then rebase this PR on that once merged. I don't mind delaying this PR until that happens as I'm not urgently waiting for this feature or anything like that.

@benscobie

benscobie commented Feb 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

This doesn't end up actually resulting in anything bad happening though, since one of the React effects causes it to re-parse the firstVal type, resetting the rule type back to the correct value...

That's another area I'd like to get cleaned up, there's a ton of inappropriate uses of useEffect that make changing things really hard.

@benscobie benscobie force-pushed the bitnimble/feat-list-count branch from 4975b19 to 110c7dc Compare March 27, 2025 00:26
@benscobie benscobie force-pushed the bitnimble/feat-list-count branch from 110c7dc to ac40ce3 Compare March 31, 2025 21:53
@benscobie benscobie enabled auto-merge (squash) March 31, 2025 22:43
benscobie
benscobie approved these changes Mar 31, 2025
View reviewed changes

@benscobie benscobie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚢 it

@benscobie benscobie merged commit a321cbd into Maintainerr:main Mar 31, 2025
@jorenn92

jorenn92 commented Apr 1, 2025

Copy link
Copy Markdown
Member

🎉 This PR is included in version 2.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@bitnimble bitnimble mentioned this pull request Apr 3, 2025
Merged
5 tasks
@enoch85 enoch85 mentioned this pull request May 25, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@benscobie benscobie benscobie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bitnimble @benscobie @jorenn92