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: Add templateid column in program rule action [DHIS2-17515] #17723

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
756b361
add programnotificationtemplate column in programruleaction
zubaira Jun 6, 2024
94df803
fix test
zubaira Jun 6, 2024
8f1f8db
fix test
zubaira Jun 6, 2024
79bb00b
fix test
zubaira Jun 6, 2024
463c6df
fix api test
zubaira Jun 6, 2024
227104a
fix integration test
zubaira Jun 6, 2024
20b56fb
fix controller test
zubaira Jun 6, 2024
d8d31ce
unit test for notification rule action validator
zubaira Jun 11, 2024
1dbed50
Merge branch 'master' into DHIS2-17515
zubaira Jun 14, 2024
5f90766
code style
zubaira Jun 17, 2024
210f479
Merge branch 'master' into DHIS2-17515
zubaira Jun 17, 2024
afb0953
assert Error
zubaira Jun 18, 2024
0f3b7f9
Merge branch 'master' into DHIS2-17515
zubaira Jun 24, 2024
d9d276e
refactor to avoid web api changes
zubaira Jun 24, 2024
94a894d
minor
zubaira Jun 24, 2024
7836f76
fix test
zubaira Jun 24, 2024
bf6ed7e
rename flyway
zubaira Jun 24, 2024
96c1355
fix test
zubaira Jun 25, 2024
ad5acfa
fix integration test
zubaira Jun 25, 2024
8e6fb00
Merge branch 'master' into DHIS2-17515
zubaira Jul 1, 2024
53e0a1a
Merge branch 'master' into DHIS2-17515
zubaira Jul 2, 2024
7499ad4
fix test
zubaira Jul 2, 2024
ea8fc4c
fix sonar cloud
zubaira Jul 2, 2024
21dcce9
fix integration test
zubaira Jul 2, 2024
ef53bf9
minor
zubaira Jul 3, 2024
ba5dab7
depricated field
zubaira Jul 3, 2024
697b189
minor
zubaira Jul 3, 2024
587d951
minor
zubaira Jul 3, 2024
3b2aa26
minor
zubaira Jul 3, 2024
2e93ca0
minor
zubaira Jul 3, 2024
772dd14
fix unit test
zubaira Jul 3, 2024
82efeda
Merge branch 'master' into DHIS2-17515
zubaira Jul 5, 2024
7de5c28
add validation for templateUid and ProgramNotificationTemplate
zubaira Jul 5, 2024
9939784
add deprecation log message
zubaira Jul 5, 2024
9cc579b
idempotent script
zubaira Jul 5, 2024
59a1d85
minor
zubaira Jul 8, 2024
9414be5
parameterized type
zubaira Jul 8, 2024
619cffb
remove unused list
zubaira Jul 8, 2024
84eeca2
remove unused list
zubaira Jul 8, 2024
d3e959e
property annotation to set owner
zubaira Jul 10, 2024
870573e
msa
zubaira Jul 10, 2024
4d01e35
remove unused import
zubaira Jul 10, 2024
fb92d5e
check preheater and then database
zubaira Jul 11, 2024
66f95b9
not to use deprecation approach
zubaira Jul 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
*/
package org.hisp.dhis.programrule;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper;
Expand All @@ -43,6 +44,8 @@
import org.hisp.dhis.program.ProgramIndicator;
import org.hisp.dhis.program.ProgramStage;
import org.hisp.dhis.program.ProgramStageSection;
import org.hisp.dhis.program.notification.ProgramNotificationTemplate;
import org.hisp.dhis.schema.annotation.Property;
import org.hisp.dhis.trackedentity.TrackedEntityAttribute;
import org.hisp.dhis.translation.Translatable;

Expand Down Expand Up @@ -134,6 +137,15 @@ public class ProgramRuleAction extends BaseIdentifiableObject implements Metadat
* <li>sendmessage
* </ul>
*/
@JsonIgnore private ProgramNotificationTemplate notificationTemplate;

/**
* The program notification template uid that will be triggered by the rule action. Used for:
*
* <ul>
* <li>sendmessage
* </ul>
*/
private String templateUid;

/**
Expand Down Expand Up @@ -322,12 +334,29 @@ public void setDataElement(DataElement dataElement) {

@JsonProperty
@JacksonXmlProperty(namespace = DxfNamespaces.DXF_2_0)
@Property(required = Property.Value.FALSE, owner = Property.Value.TRUE)
public String getTemplateUid() {
return templateUid;
return notificationTemplate != null ? notificationTemplate.getUid() : null;
}

public void setTemplateUid(String templateUid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if both templateUid and programNotificationTemplate are defined and they have different values?
Who is the winner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

programNotificationTemplate will be ignored if it's there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are not changing API.

if (templateUid == null) {
notificationTemplate = null;
return;
}

this.templateUid = templateUid;
ProgramNotificationTemplate template = new ProgramNotificationTemplate();
template.setUid(templateUid);
setNotificationTemplate(template);
}

public ProgramNotificationTemplate getNotificationTemplate() {
return notificationTemplate;
}

public void setTemplateUid(String programNotificationTemplate) {
this.templateUid = programNotificationTemplate;
public void setNotificationTemplate(ProgramNotificationTemplate notificationTemplate) {
this.notificationTemplate = notificationTemplate;
}

@JsonProperty("trackedEntityAttribute")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ summary_sql: >-
SELECT uid,name from programrule where programruleid IN (
SELECT programruleid from programruleaction WHERE
actiontype IN ('SENDMESSAGE','SCHEDULEMESSAGE')
AND templateuid IS NULL ))
AND notificationtemplateid IS NULL ))
SELECT COUNT(*) as value,
100.0 * COUNT(*) / NULLIF( (SELECT COUNT(*) FROM programrule),0) as percent
FROM program_rules_no_message_template;
details_sql: >-
SELECT uid,name from programrule where programruleid IN (
SELECT programruleid from programruleaction WHERE
actiontype IN ('SENDMESSAGE','SCHEDULEMESSAGE')
AND templateuid IS NULL );
AND notificationtemplateid IS NULL );
severity: SEVERE
introduction: >
Program rule actions of type "Send message" or "Schedule message"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ public ProgramRuleActionValidationResult validate(
ProgramRule rule = validationContext.getProgramRule();

if (!programRuleAction.hasNotification()) {
log.debug(
String.format(
"ProgramNotificationTemplate cannot be null for program rule: %s ", rule.getName()));
log.debug("templateUid cannot be null for program rule: {}", rule.getName());

return ProgramRuleActionValidationResult.builder()
.valid(false)
Expand All @@ -73,9 +71,9 @@ public ProgramRuleActionValidationResult validate(

if (pnt == null) {
log.debug(
String.format(
"ProgramNotificationTemplate id: %s for program rule: %s does not exist",
programRuleAction.getTemplateUid(), rule.getName()));
"ProgramNotificationTemplate id: {} for program rule: {} does not exist.",
programRuleAction.getTemplateUid(),
rule.getName());

return ProgramRuleActionValidationResult.builder()
.valid(false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public List<ProgramRuleAction> getProgramActionsWithNoDataObject() {
@Override
public List<ProgramRuleAction> getProgramActionsWithNoNotification() {
return getQuery(
"FROM ProgramRuleAction pra WHERE pra.programRuleActionType IN ( :notificationTypes ) AND pra.templateUid IS NULL")
"FROM ProgramRuleAction pra WHERE pra.programRuleActionType IN ( :notificationTypes ) AND pra.notificationTemplate IS NULL")
.setParameter("notificationTypes", ProgramRuleActionType.NOTIFICATION_LINKED_TYPES)
.getResultList();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@
<many-to-one name="programStage" class="org.hisp.dhis.program.ProgramStage"
column="programstageid" foreign-key="fk_programruleaction_programstage" />

<property name="templateUid" column="templateuid"/>
<many-to-one name="notificationTemplate" class="org.hisp.dhis.program.notification.ProgramNotificationTemplate"
column="notificationtemplateid" foreign-key="fk_programruleaction_notificationtemplate" not-null="false" />

<many-to-one name="option" class="org.hisp.dhis.option.Option"
column="optionid" foreign-key="fk_programruleaction_option" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@
import org.hisp.dhis.program.Program;
import org.hisp.dhis.program.ProgramService;
import org.hisp.dhis.program.ProgramStage;
import org.hisp.dhis.program.notification.NotificationTrigger;
import org.hisp.dhis.program.notification.ProgramNotificationRecipient;
import org.hisp.dhis.program.notification.ProgramNotificationTemplate;
import org.hisp.dhis.programrule.action.validation.HideOptionProgramRuleActionValidator;
import org.hisp.dhis.programrule.action.validation.NotificationProgramRuleActionValidator;
import org.hisp.dhis.programrule.action.validation.ProgramRuleActionValidationContext;
import org.hisp.dhis.programrule.action.validation.ProgramRuleActionValidationService;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -76,13 +80,13 @@ void setUp() {
.programStages(List.of(programStage))
.programRuleActionValidationService(programRuleActionValidationService)
.build();
when(programRuleActionValidationService.getProgramService()).thenReturn(programService);
when(programService.getProgram(any())).thenReturn(program);
}

@Test
@DisplayName("Should return valid when HideOptionProgramRule contains DataElement")
void testHideOptionRuleValid() {
when(programRuleActionValidationService.getProgramService()).thenReturn(programService);
when(programService.getProgram(any())).thenReturn(program);
Option option = new Option();
ProgramRuleAction programRuleAction = new ProgramRuleAction();
programRuleAction.setDataElement(dataElement);
Expand All @@ -102,6 +106,8 @@ void testHideOptionRuleValid() {
@Test
@DisplayName("Should return error E4058 when HideOptionProgramRule contains ProgramStage")
void testHideOptionInvalid() {
when(programRuleActionValidationService.getProgramService()).thenReturn(programService);
when(programService.getProgram(any())).thenReturn(program);
Option option = new Option();
ProgramRuleAction programRuleAction = new ProgramRuleAction();
programRuleAction.setDataElement(dataElement);
Expand All @@ -119,4 +125,50 @@ void testHideOptionInvalid() {
assertFalse(result.isValid());
assertEquals(ErrorCode.E4058, result.getErrorReport().getErrorCode());
}

@Test
void testValidRuleActionSendMessage() {
ProgramNotificationTemplate pnt =
createProgramNotificationTemplate(
"test", 1, NotificationTrigger.PROGRAM_RULE, ProgramNotificationRecipient.USER_GROUP);

ProgramRuleAction programRuleAction = new ProgramRuleAction();
programRuleAction.setTemplateUid(pnt.getUid());
programRuleAction.setProgramRuleActionType(ProgramRuleActionType.SENDMESSAGE);

ProgramRule programRule = new ProgramRule();
programRule.setProgram(program);
programRule.setName("Test Program Rule");
programRule.getProgramRuleActions().add(programRuleAction);

validationContext.setProgramRule(programRule);
validationContext.setNotificationTemplate(pnt);

NotificationProgramRuleActionValidator notificationValidator =
new NotificationProgramRuleActionValidator();
ProgramRuleActionValidationResult result =
notificationValidator.validate(programRuleAction, validationContext);
assertTrue(result.isValid());
}

@Test
void testInValidRuleActionSendMessage() {
ProgramRuleAction programRuleAction = new ProgramRuleAction();
programRuleAction.setProgramRuleActionType(ProgramRuleActionType.SENDMESSAGE);

ProgramRule programRule = new ProgramRule();
programRule.setProgram(program);
programRule.setName("Test Program Rule");
programRule.getProgramRuleActions().add(programRuleAction);

validationContext.setProgramRule(programRule);
validationContext.setNotificationTemplate(null);

NotificationProgramRuleActionValidator notificationValidator =
new NotificationProgramRuleActionValidator();
ProgramRuleActionValidationResult result =
notificationValidator.validate(programRuleAction, validationContext);
assertFalse(result.isValid());
assertEquals(ErrorCode.E4035, result.getErrorReport().getErrorCode());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@
import java.util.Map;
import java.util.function.Consumer;
import javax.annotation.Nonnull;
import lombok.extern.slf4j.Slf4j;
import org.hisp.dhis.dxf2.metadata.objectbundle.ObjectBundle;
import org.hisp.dhis.feedback.ErrorReport;
import org.hisp.dhis.program.notification.ProgramNotificationTemplate;
import org.hisp.dhis.programrule.ProgramRuleAction;
import org.hisp.dhis.programrule.ProgramRuleActionType;
import org.hisp.dhis.programrule.ProgramRuleActionValidationResult;
Expand All @@ -44,6 +46,7 @@
* @author Zubair Asghar
*/
@Component("programRuleActionObjectBundle")
@Slf4j
public class ProgramRuleActionObjectBundleHook extends AbstractObjectBundleHook<ProgramRuleAction> {
@Nonnull
private final Map<ProgramRuleActionType, ProgramRuleActionValidator>
Expand All @@ -69,6 +72,17 @@ public void validate(
}
}

@Override
public void preCreate(ProgramRuleAction object, ObjectBundle bundle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we getting ProgramNotificationTemplate from the DB?
Should be already in ProgramRuleAction at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It will be ProgramNotificationTemplate object with only uid.

getNotificationTemplate(object, bundle);
}

@Override
public void preUpdate(
ProgramRuleAction object, ProgramRuleAction persistedObject, ObjectBundle bundle) {
getNotificationTemplate(object, bundle);
}

private ProgramRuleActionValidationResult validateProgramRuleAction(
ProgramRuleAction ruleAction, ObjectBundle bundle) {
ProgramRuleActionValidationResult validationResult;
Expand All @@ -83,4 +97,32 @@ private ProgramRuleActionValidationResult validateProgramRuleAction(

return validationResult;
}

private void getNotificationTemplate(ProgramRuleAction object, ObjectBundle bundle) {
if (object.getTemplateUid() == null) {
// Nothing to do when templateUid is null
return;
}

// try to get the template from the preheated bundle.
ProgramNotificationTemplate template =
bundle
.getPreheat()
.get(
bundle.getPreheatIdentifier(),
ProgramNotificationTemplate.class,
object.getTemplateUid());

// If not found in preheated objects, fetch from the database.
if (template == null) {
template = manager.get(ProgramNotificationTemplate.class, object.getTemplateUid());
}

if (template == null) {
log.info("No ProgramNotificationTemplate found for {}: ", object.getTemplateUid());
return;
}

object.setNotificationTemplate(template);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
-- Add the new column
ALTER TABLE programruleaction ADD COLUMN IF NOT EXISTS notificationtemplateid INTEGER NULL;

-- Update the new column with values from the old column
UPDATE programruleaction pra
SET notificationtemplateid = t.programnotificationtemplateid
FROM programnotificationtemplate t
WHERE pra.templateuid = t.uid;

-- Remove templateUid

ALTER TABLE programruleaction DROP COLUMN IF EXISTS templateuid;

-- Conditionally add the foreign key constraint if it doesn't exist
DO $$
BEGIN
IF NOT EXISTS (
SELECT 1
FROM pg_constraint
WHERE conname = 'fk_notificationtemplate'
) THEN
ALTER TABLE programruleaction
ADD CONSTRAINT fk_notificationtemplate
FOREIGN KEY (notificationtemplateid) REFERENCES programnotificationtemplate(programnotificationtemplateid);
END IF;
END $$;
Original file line number Diff line number Diff line change
Expand Up @@ -2284,8 +2284,13 @@ public static Constant createConstant(char uniqueCharacter, double value) {

public static ProgramNotificationTemplate createProgramNotificationTemplate(
String name, int days, NotificationTrigger trigger, ProgramNotificationRecipient recipient) {
return new ProgramNotificationTemplate(
name, "Subject", "Message", trigger, recipient, Sets.newHashSet(), days, null, null);
ProgramNotificationTemplate template =
new ProgramNotificationTemplate(
name, "Subject", "Message", trigger, recipient, Sets.newHashSet(), days, null, null);

template.setAutoFields();

return template;
}

public static ProgramNotificationTemplate createProgramNotificationTemplate(
Expand All @@ -2294,8 +2299,12 @@ public static ProgramNotificationTemplate createProgramNotificationTemplate(
NotificationTrigger trigger,
ProgramNotificationRecipient recipient,
Date scheduledDate) {
return new ProgramNotificationTemplate(
name, "Subject", "Message", trigger, recipient, Sets.newHashSet(), days, null, null);
ProgramNotificationTemplate template =
new ProgramNotificationTemplate(
name, "Subject", "Message", trigger, recipient, Sets.newHashSet(), days, null, null);
template.setAutoFields();

return template;
}

public static DataSetNotificationTemplate createDataSetNotificationTemplate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@
import org.hisp.dhis.program.ProgramService;
import org.hisp.dhis.program.ProgramStage;
import org.hisp.dhis.program.ProgramStageService;
import org.hisp.dhis.program.notification.NotificationTrigger;
import org.hisp.dhis.program.notification.ProgramNotificationRecipient;
import org.hisp.dhis.program.notification.ProgramNotificationTemplate;
import org.hisp.dhis.program.notification.ProgramNotificationTemplateService;
import org.hisp.dhis.test.integration.TransactionalIntegrationTest;
import org.hisp.dhis.trackedentity.TrackedEntityAttribute;
import org.hisp.dhis.trackedentity.TrackedEntityAttributeService;
Expand Down Expand Up @@ -76,6 +80,8 @@ class ProgramRuleActionServiceTest extends TransactionalIntegrationTest {

@Autowired private ProgramStageService programStageService;

@Autowired private ProgramNotificationTemplateService programNotificationTemplateService;

@Override
public void setUpTest() {
programStageA = createProgramStage('A', 0);
Expand Down Expand Up @@ -332,7 +338,18 @@ void testProgramRuleActionWithNoNotification() {
"$placeofliving",
null,
null);
actionI.setTemplateUid("tempUId");

ProgramNotificationTemplate pnt =
createProgramNotificationTemplate(
"test123",
3,
NotificationTrigger.PROGRAM_RULE,
ProgramNotificationRecipient.USER_GROUP);

programNotificationTemplateService.save(pnt);

actionI.setTemplateUid(pnt.getUid());
actionI.setNotificationTemplate(pnt);
actionService.addProgramRuleAction(actionI);
actionService.addProgramRuleAction(actionJ);
programRuleA.setProgramRuleActions(Sets.newHashSet(actionI, actionJ));
Expand Down
Loading
Loading