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

chore: let COC.isDefault look at its own name only DHIS2-14968 #19266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

teleivo
Copy link
Contributor

@teleivo teleivo commented Nov 22, 2024

discovered while implementing #19252

Category option combos are not always fetched using hibernate. In tracker we use jdbc when fetching events and only fetch and map what's needed. We thus do not get the category combo. This means that coc.isDefault() returns false even if it is the default coc.

We want to change isDefault

  @Override
  public boolean isDefault() {
-    return categoryCombo != null && DEFAULT_NAME.equals(categoryCombo.getName());
+    DEFAULT_NAME.equals(name);
  }

Default category/option/combo are created on startup

return getCategoryComboByName(CategoryCombo.DEFAULT_CATEGORY_COMBO_NAME);

The category option name is unique and the system "takes" the default name. The coc is generated from the co and so is its name which is default.

When retrieving the default coc from the service it already relies on the name default

public CategoryOptionCombo getDefaultCategoryOptionCombo() {
return categoryOptionComboStore.getByName(CategoryCombo.DEFAULT_CATEGORY_COMBO_NAME);
}

Copy link

sonarcloud bot commented Nov 26, 2024

@teleivo teleivo marked this pull request as ready for review November 26, 2024 06:07
@teleivo teleivo requested a review from a team as a code owner November 26, 2024 06:07
Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

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

As Jim mentioned there is code using the wrong constant for the string "default" because they all are the same but good that you cleaned that up as well. It is a bit confusing and I assume was just a copy&paste or autocomplete mistake made in the past.

@jason-p-pickering
Copy link
Contributor

Hmm. Is this the only way to solve this problem? I see that all of the tests pass, but I do worry about changing this for this particular reason. What happens when there are multiple default COCs? I have been working on a PR here to prevent import of new/duplicate COCs, and we have some integrity checks around this as well, but we do know of systems which either have a default CC/CO/COC which are not the hard-coded ones which we currently have, or which have actual duplicate default COCs (i.e multiple "default" COCs) which are associated with the default CC.

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.

4 participants