filter eCRF forms also by a field's external_id#404
Conversation
WalkthroughThe changes refactor ECRF field search logic and input field naming by introducing external ID support. Search criteria now include externalId fields with explicit null/empty guards. Input field models transition from public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InquiryInputModel.java (1)
249-255: Remove commented-out code or deprecate the method properly.Same issue as in ProbandListEntryTagInputModel.java: commented-out code should not remain in production. Either remove this entirely or properly deprecate with
@Deprecatedannotation.Apply this diff to remove the commented code:
- // @Override - // public String getUniqueName() { - // if (inquiry != null) { - // return inquiry.getUniqueName(); - // } - // return null; - // }Alternatively, if deprecation is needed:
- // @Override - // public String getUniqueName() { - // if (inquiry != null) { - // return inquiry.getUniqueName(); - // } - // return null; - // } + @Override + @Deprecated + public String getUniqueName() { + // Deprecated: Use getName() which now incorporates external ID + if (inquiry != null) { + return inquiry.getUniqueName(); + } + return null; + }
🧹 Nitpick comments (6)
core/src/main/java/org/phoenixctms/ctsms/domain/ECRFFieldValueDaoImpl.java (2)
123-127: Simplify the double-nested AND and reconsider the guard logic.The
Restrictions.and(Restrictions.and(...))nesting on lines 123-124 is unnecessary and reduces readability. Additionally, the guard requires BOTHexternalIdandinputField0.externalIdto be empty/null before searchinginputField0.nameL10nKey. This might be overly restrictive—consider whether the intended behavior is to searchnameL10nKeywheneverinputField0.externalIdis empty, regardless of the parentexternalId.Apply this diff to flatten the nested AND:
-junction.add(Restrictions.and( - Restrictions.and( - Restrictions.or(Restrictions.eq("externalId", ""), Restrictions.isNull("externalId")), - Restrictions.or(Restrictions.eq("inputField0.externalId", ""), Restrictions.isNull("inputField0.externalId"))), - Restrictions.ilike("inputField0.nameL10nKey", fieldQuery, MatchMode.ANYWHERE))); +junction.add(Restrictions.and( + Restrictions.or(Restrictions.eq("externalId", ""), Restrictions.isNull("externalId")), + Restrictions.or(Restrictions.eq("inputField0.externalId", ""), Restrictions.isNull("inputField0.externalId")), + Restrictions.ilike("inputField0.nameL10nKey", fieldQuery, MatchMode.ANYWHERE)));
122-133: Consider extracting guard logic into helper methods.The repeated pattern of checking for null/empty fields (e.g.,
Restrictions.or(Restrictions.eq("externalId", ""), Restrictions.isNull("externalId"))) appears multiple times. Extracting this into a helper method would improve maintainability and reduce duplication.For example, add a helper method:
private static Junction isNullOrEmpty(String propertyName) { return Restrictions.or( Restrictions.eq(propertyName, ""), Restrictions.isNull(propertyName) ); }Then refactor the guards:
-junction.add(Restrictions.and( - Restrictions.or(Restrictions.eq("externalId", ""), Restrictions.isNull("externalId")), - Restrictions.ilike("inputField0.externalId", fieldQuery, MatchMode.ANYWHERE))); +junction.add(Restrictions.and( + isNullOrEmpty("externalId"), + Restrictions.ilike("inputField0.externalId", fieldQuery, MatchMode.ANYWHERE)));web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldOutVOConfig.java (1)
29-32: Commented code for future reference.The commented-out
getExternalId()override appears to be a placeholder for potential future functionality. While this documents the intent, consider whether these commented blocks should be tracked as TODOs or removed if not needed in the near term.web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldConfig.java (1)
15-15: Commented abstract method declaration.Similar to other files in this PR, the commented-out abstract method serves as documentation but doesn't affect functionality. Consider consolidating these placeholder comments or tracking them through issue management.
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputModel.java (1)
359-359: Avoid commented-out public API; either remove it fully or deprecate properly
//public abstract String getUniqueName();looks like a half-step and will rot. If migration is done, delete it; if not, consider a temporary@Deprecatedconcrete method (delegating to the old behavior) to make the transition explicit.web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/DummyInputModel.java (1)
186-189: Remove commented-outgetUniqueName()blockSince the method is no longer part of the contract, keeping the commented override is just noise and makes future refactors harder.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/src/main/java/org/phoenixctms/ctsms/domain/ECRFFieldValueDaoImpl.java(1 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/DummyInputModel.java(2 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/EcrfFieldInputModel.java(3 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldConfig.java(1 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldInVOConfig.java(2 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldOutVOConfig.java(1 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldOutVOConfigBase.java(2 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputModel.java(2 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InquiryDummyInputModel.java(1 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InquiryInputModel.java(3 hunks)web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/ProbandListEntryTagInputModel.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldInVOConfig.java (1)
common/src/main/java/org/phoenixctms/ctsms/util/CommonUtil.java (1)
CommonUtil(113-2756)
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputModel.java (1)
common/src/main/java/org/phoenixctms/ctsms/util/CommonUtil.java (1)
CommonUtil(113-2756)
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldOutVOConfigBase.java (1)
common/src/main/java/org/phoenixctms/ctsms/util/CommonUtil.java (1)
CommonUtil(113-2756)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (12)
core/src/main/java/org/phoenixctms/ctsms/domain/ECRFFieldValueDaoImpl.java (1)
117-136: Verify the search behavior with integration tests.The new search logic introduces complex precedence rules with multiple guards. Ensure that all intended search paths are covered by tests, including edge cases where various combinations of fields are null/empty.
Consider adding test cases for:
- Searching when
ECRFField.externalIdis set vs. empty- Searching when
InputField.externalIdis set vs. empty- Searching when both
externalIdfields are set/empty- Verifying that
nameL10nKeyis searched only when bothexternalIdfields are empty- Verifying that
inputField0.titleL10nKeyis searched whentitleL10nKeyis emptyweb/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldOutVOConfigBase.java (1)
19-27: LGTM: External ID preference logic is correct.The getName() implementation properly handles null checks and uses
CommonUtil.isEmptyString()to determine whether to prefer the external ID over the field name. This aligns with the PR's objective to enable filtering by external ID.web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldInVOConfig.java (1)
28-35: LGTM: Consistent external ID preference implementation.The getName() logic matches the pattern established in
InputFieldOutVOConfigBase, correctly usingCommonUtil.isEmptyString()to determine whether to prefer the external ID.web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/EcrfFieldInputModel.java (2)
168-177: LGTM: Message construction now uses external ID-aware naming.The change from
inputField.getName()tosuper.getName()ensures that the message titles will use the external ID when available, consistent with the PR's objective.
647-653: LGTM: External ID accessor correctly implemented.The
getInputExternalId()override properly checks for null before accessingecrfField.getExternalId().web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InquiryDummyInputModel.java (1)
28-30: LGTM: Simplified delegation to parent class.The getName() implementation now consistently delegates to the parent class with the appropriate message code. This aligns with the broader refactoring to centralize external ID-aware naming logic.
Note: This depends on a
getName(String)overload inInquiryInputModel(mentioned in the AI summary but not included in this review).web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/ProbandListEntryTagInputModel.java (2)
551-558: LGTM!The new
getInputExternalId()method correctly provides external ID access with proper null safety. The protected visibility is appropriate for this hook method.
185-187: [rewritten comment]
[classification tag]web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InquiryInputModel.java (3)
163-172: LGTM!The new protected
getName(String newInquiryValueInputTitleCode)method is a good refactoring that parameterizes the message code and reduces duplication. Usingsuper.getName()correctly delegates name resolution to the superclass.
174-177: LGTM!The delegation pattern is clean and maintains the public API contract while consolidating the implementation logic.
534-541: LGTM!The new
getInputExternalId()method correctly implements the external ID hook pattern with proper null safety. The implementation is consistent with the parallel change in ProbandListEntryTagInputModel.java.web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/DummyInputModel.java (1)
395-398:getInputExternalId()returning null is fine for a dummy model—verify it won’t break externalId-based filteringIf dummy inputs can appear in the filtered collections, ensure the new externalId-aware search logic handles null external IDs as intended (no accidental matches / no NPEs).
| protected abstract String getInputExternalId(); | ||
|
|
||
| public String getName() { | ||
| String externalId = getInputExternalId(); | ||
| if (CommonUtil.isEmptyString(externalId)) { | ||
| return super.getName(); | ||
| } else { | ||
| return externalId; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and examine the class definition and parent class
find . -name "InputModel.java" -type f | head -5Repository: phoenixctms/ctsms
Length of output: 146
🏁 Script executed:
# Get the full context around the methods mentioned
head -n 350 web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputModel.java | tail -n 150Repository: phoenixctms/ctsms
Length of output: 5361
🏁 Script executed:
# Search for all usages of getName() on InputModel instances
rg "\.getName\(\)" --type java -B 2 -A 2 | head -100Repository: phoenixctms/ctsms
Length of output: 11630
🏁 Script executed:
# Find the parent class of InputModel
head -n 50 web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputModel.java | grep -E "class InputModel|extends|implements"Repository: phoenixctms/ctsms
Length of output: 129
🏁 Script executed:
# Search specifically for InputModel usages with getName()
rg "InputModel" --type java -B 3 -A 3 | grep -A 5 -B 5 "getName" | head -80Repository: phoenixctms/ctsms
Length of output: 43
🏁 Script executed:
# Search for map/collection operations on InputModel that might use getName as a key
rg "InputModel" --type java -B 2 -A 2 | grep -E "(put|get|key|Map|contains|indexOf)" | head -40Repository: phoenixctms/ctsms
Length of output: 5271
🏁 Script executed:
# Find InputFieldOutVOConfigBase class
find . -name "InputFieldOutVOConfigBase.java" -type fRepository: phoenixctms/ctsms
Length of output: 161
🏁 Script executed:
# Examine the parent class
cat -n web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldOutVOConfigBase.java | head -100Repository: phoenixctms/ctsms
Length of output: 1712
🏁 Script executed:
# Search for getName() being used specifically in context where it matters as an identifier
rg "getName\(\)" --type java -B 5 -A 1 | grep -E "(Map|put|get|Set|contains|key|id)" -B 8 -A 2 | head -100Repository: phoenixctms/ctsms
Length of output: 13001
🏁 Script executed:
# Find InputFieldConfig class
find . -name "InputFieldConfig.java" -type fRepository: phoenixctms/ctsms
Length of output: 152
🏁 Script executed:
# Check what InputFieldConfig defines
cat -n web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldConfig.java | head -80Repository: phoenixctms/ctsms
Length of output: 3062
🏁 Script executed:
# Verify the exact override chain and whether InputModel.getName() is new
# Look at entire InputModel class to understand structure
wc -l web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputModel.javaRepository: phoenixctms/ctsms
Length of output: 148
🏁 Script executed:
# Check the git context to see if this is truly new behavior or refactoring
# Look for any comments or annotations in InputModel around line 230-239
sed -n '220,250p' web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputModel.javaRepository: phoenixctms/ctsms
Length of output: 722
🏁 Script executed:
# Check InputModel subclasses to see if they override getInputExternalId()
rg "getInputExternalId" --type java -B 2 -A 5Repository: phoenixctms/ctsms
Length of output: 4535
🏁 Script executed:
# Final check: confirm parent class uses inputField directly
grep -A 8 "public String getName()" web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputFieldOutVOConfigBase.javaRepository: phoenixctms/ctsms
Length of output: 374
🏁 Script executed:
# Check if there's any usage of InputModel.getName() in templates or JS context where it might be used as ID
rg "getName\(\)" --type java web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/ -B 3 -A 3 | grep -E "(input|id|select|radio|checkbox|component)" -iRepository: phoenixctms/ctsms
Length of output: 16840
Add @Override annotation to getName() method for compile-time safety
The method is overriding a parent class method but lacks the @Override annotation. Note that the externalId-preferring behavior already existed in the parent class InputFieldOutVOConfigBase, so this refactoring extracts externalId retrieval to an abstract hook (getInputExternalId()) allowing subclasses to provide their own sources—a design improvement rather than a breaking change.
- public String getName() {
+ @Override
+ public String getName() {
String externalId = getInputExternalId();
if (CommonUtil.isEmptyString(externalId)) {
return super.getName();
} else {
return externalId;
}
}🤖 Prompt for AI Agents
In
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/InputModel.java
around lines 230 to 239, the getName() method overrides a superclass method but
lacks the @Override annotation; add the @Override annotation immediately above
the getName() method declaration (keeping the existing logic and signature) to
ensure compile-time checking that it correctly overrides the parent method.
| // @Override | ||
| // public String getUniqueName() { | ||
| // if (tag != null) { | ||
| // return tag.getUniqueName(); | ||
| // } | ||
| // return null; | ||
| // } |
There was a problem hiding this comment.
Remove commented-out code or deprecate the method properly.
Commented-out code should not remain in production. If getUniqueName() is being removed as part of the API transition to external IDs, it should be either:
- Removed entirely, or
- Deprecated with
@Deprecatedannotation and a clear migration path in the javadoc
The commented-out code creates ambiguity and technical debt.
Apply this diff to remove the commented code:
- // @Override
- // public String getUniqueName() {
- // if (tag != null) {
- // return tag.getUniqueName();
- // }
- // return null;
- // }Alternatively, if this needs a deprecation period, apply this diff instead:
- // @Override
- // public String getUniqueName() {
- // if (tag != null) {
- // return tag.getUniqueName();
- // }
- // return null;
- // }
+ @Override
+ @Deprecated
+ public String getUniqueName() {
+ // Deprecated: Use getName() which now incorporates external ID
+ if (tag != null) {
+ return tag.getUniqueName();
+ }
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @Override | |
| // public String getUniqueName() { | |
| // if (tag != null) { | |
| // return tag.getUniqueName(); | |
| // } | |
| // return null; | |
| // } |
| // @Override | |
| // public String getUniqueName() { | |
| // if (tag != null) { | |
| // return tag.getUniqueName(); | |
| // } | |
| // return null; | |
| // } | |
| @Override | |
| @Deprecated | |
| public String getUniqueName() { | |
| // Deprecated: Use getName() which now incorporates external ID | |
| if (tag != null) { | |
| return tag.getUniqueName(); | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
web/src/main/java/org/phoenixctms/ctsms/web/model/shared/inputfield/ProbandListEntryTagInputModel.java
around lines 274-280, remove the commented-out getUniqueName() implementation to
avoid leaving dead code; either delete these commented lines entirely, or if a
deprecation period is required, restore the method as a real method annotated
with @Deprecated and add a Javadoc explaining the migration to external IDs
(point to the new getter to use and any timeline), then run formatter/compile to
ensure no unused imports or warnings remain.
1 participant
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.