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

feat: limits for category combinations [DHIS2-18521] #19374

Merged
merged 5 commits into from
Dec 5, 2024
Merged

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Dec 3, 2024

Summary

Adds 3 configurations for new limits around categories that are validated on create and update.

  • metadata.categories.max_options: The maximum number of category options in a single category
  • metadata.categories.max_per_combo: The maximum number of categories per category combo
  • metadata.categories.max_combinations: The maximum for the product of the number of options in a category combo

Automatic Testing

New controller tests added for each of the validations.

Manual Testing

Test limit max categories in a combo:

  • goto maintenance app
  • create a new category combo
  • enter a name, select 6 categories and press save
  • check a error message is shown that explains that a combo can only have 5 categories

Test limit max options per category:

  • goto maintenance app
  • edit an existing category
  • select more than 31 options (move to right) and press save
  • check a error message is shown that explains that a category can only have 31 options or less

Test limit max combinations:

  • goto maintenance app
  • find or create 3 categories with each have at least 8 (or more) options
  • create a category combo with these 3 categories and press save
  • check a error message is shown that explains that a category combo can only have more than 500 combinations

Documentation

PR dhis2/dhis2-docs#1464

@jbee jbee self-assigned this Dec 3, 2024
Copy link

sonarcloud bot commented Dec 4, 2024

@@ -58,8 +59,10 @@ private DeletionVeto allowDeleteCategory(Category category) {
}

private void deleteCategoryOptionCombo(CategoryOptionCombo categoryOptionCombo) {
for (CategoryCombo categoryCombo : categoryService.getAllCategoryCombos()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: I am not sure why we would loop over all CCs when there is just one linked to the COC which needs unlinking. Also I added a check if the un-linking isn't already done because if this was called as a consequence of deleting the CC it has and so the update should not be called again on the CC.

Copy link
Contributor

Choose a reason for hiding this comment

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

If not needed, this is very nice. That could be very costly.
An existing/new test would confirm that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I only have my subjective observation from test and debugging but I found this because an deletion attempt would just spin my PC out of control so I had to kill the server, then step through it to see what was going on, which was when I found this. I then redid the deletion and it still was super slow but the server was at least responsive.

@jbee jbee marked this pull request as ready for review December 4, 2024 15:59
@@ -69,6 +69,10 @@ public enum ErrorCode {

E1122("Category option combo {0} cannot be associated with the default category combo"),
E1125("Category option combo {0} contains options not associated with category combo {1}"),
E1126("Category combo {0} cannot combine more than {1} categories, but had: {2}"),
E1127("Category {0} cannot have more than {1} options, but had: {2} "),
E1128("Category combo {0} cannot have more than {1} combinations, bud requires: {2}"),
Copy link
Contributor

@david-mackessy david-mackessy Dec 5, 2024

Choose a reason for hiding this comment

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

maybe this message can mirror the other 2?
..... cannot have more than x combinations, but had: y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is worded differently because it is different. The combinations do not exist (yet) and when the validation fails we also don't create them, so I thought wording it "but had" is confusing if you know what is going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok cool. There is a typo in bud. Should be but.

Copy link
Contributor

@david-mackessy david-mackessy left a comment

Choose a reason for hiding this comment

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

This sounds like a good idea, to have default limits for some items.
I'm wondering how this will play out in real use cases.
If a user tries to create more than the default, they will get the error message. Is the intention then that they should rethink how they are modelling things, but will most likely just update the config and get the higher limit they are looking for.
So it may just be an extra hoop they need to jump through (might be seen as annoying).

@jbee
Copy link
Contributor Author

jbee commented Dec 5, 2024

...but will most likely just update the config and get the higher limit they are looking for.
So it may just be an extra hoop they need to jump through (might be seen as annoying).

Yes, this usually should trigger a conversation. First to see if the use case really is what they were trying to model but if so they should ask the admins to increase the limit in the config. A good example is if you work with a extensive code standard that has thousands of codes that is a valid use case. But using the limits you can basically prevent mixing such category with another category by setting the max-options and max-combinations to same value.

@jbee jbee merged commit 17c4998 into master Dec 5, 2024
16 checks passed
@jbee jbee deleted the DHIS2-18521 branch December 5, 2024 08:42
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.

3 participants