Limit the number of attachments per form entry#3653
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an attachment-limit feature: two new UI strings, attachment counting persisted in FormEntryActivity with MAX_ATTACHMENTS = 50, and public APIs to check/increment/decrement the count and show a limit error. Widgets (Image, Media, Signature, Audio, Video, Document) implement a MediaCapableWidget interface or use it to check the limit before starting capture/chooser actions; they update the activity counter when media is added/cleared. FileUtil gains a helper to count media files in an instance directory used to initialize the counter. Sequence Diagram(s)sequenceDiagram
participant User
participant Widget as Media Widget (Image/Audio/Video/Doc/Signature)
participant Activity as FormEntryActivity
User->>Widget: Tap capture/choose
Widget->>Widget: isAttachmentLimitReached()
alt Limit Unknown -> asks Activity
Widget->>Activity: (via isAttachmentLimitReached) check canAddAttachment
Activity->>Activity: attachmentCount < MAX_ATTACHMENTS?
end
alt Limit Not Reached
Widget-->>User: Launch capture/chooser intent
User-->>Widget: Returns media
Widget->>Widget: setMediaName(...)
Widget->>Activity: incrementAttachmentCount()
Activity->>Activity: attachmentCount++
Activity-->>Widget: OK
else Limit Reached
Activity-->>Widget: showFormAttachmentLimitReachedError()
Widget-->>User: Display limit-reached message
end
sequenceDiagram
participant Activity as FormEntryActivity
participant FileUtil
participant Storage as Form Instance Dir
Activity->>FileUtil: countNumberOfMediaFilesInDirectory(dirPath)
FileUtil->>Storage: list files
FileUtil->>FileUtil: filter supported media & AES files
FileUtil-->>Activity: return mediaCount
Activity->>Activity: attachmentCount = mediaCount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/org/commcare/views/widgets/MediaWidget.java (1)
277-316:⚠️ Potential issue | 🔴 CriticalAttachment replacement blocked at limit due to check order.
The limit check on line 279 occurs before the existing media deletion on lines 285-287. If a user is at the 50-attachment limit and wants to replace an existing attachment on this question, the check will fail and show an error, even though the operation should be allowed (replacing doesn't increase the net count).
The PR objective states "Replacements (re-capturing for the same question) do not double-count because existing media is deleted first," but the current code order prevents this.
🐛 Proposed fix to allow replacement at limit
`@Override` public void setBinaryData(Object binaryURI) { + boolean isReplacement = mBinaryName != null; - if (!((FormEntryActivity)getContext()).canAddAttachment()) { + if (!isReplacement && !((FormEntryActivity)getContext()).canAddAttachment()) { notifyInvalid(StringUtils.getStringRobust(getContext(), R.string.form_attachment_limit_reached), true); return; } // delete any existing media if (mBinaryName != null) { deleteMedia(); } // ... rest of method ... mTempBinaryPath = binaryPath; mBinaryName = removeAESExtension(newMedia.getName()); - ((FormEntryActivity)getContext()).incrementAttachmentCount(); + if (!isReplacement) { + ((FormEntryActivity)getContext()).incrementAttachmentCount(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/views/widgets/MediaWidget.java` around lines 277 - 316, The attachment limit check in setBinaryData currently runs before deleting existing media, so replacements are blocked when at the limit; change the logic in setBinaryData (method) to allow replacements by either moving the canAddAttachment() check to after the existing-media deletion block (deleteMedia()) or by making the check conditional: call ((FormEntryActivity)getContext()).canAddAttachment() only when mBinaryName is null (i.e., when adding new media), otherwise skip the limit check so replacements proceed, and keep the rest of the flow (createFilePath, size/extension checks, encryptRecordedFileToDestination, updating mTempBinaryPath/mBinaryName and incrementAttachmentCount()) unchanged.app/src/org/commcare/views/widgets/ImageWidget.java (1)
297-311:⚠️ Potential issue | 🔴 CriticalSame replacement bug as MediaWidget: limit check blocks valid replacements.
The limit check on line 298 runs before the deletion on lines 305-307. At the 50-attachment limit, users cannot replace an existing image on this question, even though replacement should be allowed.
🐛 Proposed fix to allow replacement at limit
`@Override` public void setBinaryData(Object binaryPath) { - if (!((FormEntryActivity)getContext()).canAddAttachment()) { + boolean isReplacement = mBinaryName != null; + if (!isReplacement && !((FormEntryActivity)getContext()).canAddAttachment()) { notifyInvalid(StringUtils.getStringRobust(getContext(), R.string.form_attachment_limit_reached), true); return; } // you are replacing an answer. delete the previous image using the // content provider. if (mBinaryName != null) { deleteMedia(); } File f = new File(binaryPath.toString()); mBinaryName = f.getName(); - ((FormEntryActivity)getContext()).incrementAttachmentCount(); + if (!isReplacement) { + ((FormEntryActivity)getContext()).incrementAttachmentCount(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/views/widgets/ImageWidget.java` around lines 297 - 311, The attachment-limit check in setBinaryData currently runs before deletion and prevents replacing an existing image; change the logic so canAddAttachment() is only enforced when adding a new attachment (i.e., when mBinaryName is null). In setBinaryData, allow replacement by moving/adjusting the limit check to: if (mBinaryName == null && !((FormEntryActivity)getContext()).canAddAttachment()) { notifyInvalid(...); return; }, call deleteMedia() when mBinaryName != null as now, and only call ((FormEntryActivity)getContext()).incrementAttachmentCount() when this is a new attachment (mBinaryName was null). This fixes replacement at the limit while preserving the overall behavior of setBinaryData, mBinaryName, deleteMedia(), canAddAttachment(), and incrementAttachmentCount().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/org/commcare/activities/FormEntryActivity.java`:
- Around line 434-446: The attachment counter is only incremented in
ImageWidget.setAnswer()/MediaWidget.setAnswer() but never decremented; call
FormEntryActivity.decrementAttachmentCount() when an attachment is
removed—either by adding a call to decrementAttachmentCount() inside
ImageWidget.clearAnswer() and MediaWidget.clearAnswer() after deleteMedia()
succeeds, or by invoking it from deleteMedia() itself so removals always
decrement the counter; keep the existing guard in decrementAttachmentCount() to
prevent negative counts.
- Around line 185-186: Persist and restore the attachmentCount across config
changes and when loading existing forms: add a key constant (e.g.,
ATTACHMENT_COUNT_KEY), save attachmentCount in onSaveInstanceState, restore it
in loadStateFromBundle (falling back to 0 if absent), and ensure attachmentCount
is initialized when loading an existing form by counting current attachments
from the form model (use the form's attachment list / media map where
FormEntryActivity reads or populates attachments) so the counter reflects
already-saved attachments and still enforces MAX_ATTACHMENTS.
---
Outside diff comments:
In `@app/src/org/commcare/views/widgets/ImageWidget.java`:
- Around line 297-311: The attachment-limit check in setBinaryData currently
runs before deletion and prevents replacing an existing image; change the logic
so canAddAttachment() is only enforced when adding a new attachment (i.e., when
mBinaryName is null). In setBinaryData, allow replacement by moving/adjusting
the limit check to: if (mBinaryName == null &&
!((FormEntryActivity)getContext()).canAddAttachment()) { notifyInvalid(...);
return; }, call deleteMedia() when mBinaryName != null as now, and only call
((FormEntryActivity)getContext()).incrementAttachmentCount() when this is a new
attachment (mBinaryName was null). This fixes replacement at the limit while
preserving the overall behavior of setBinaryData, mBinaryName, deleteMedia(),
canAddAttachment(), and incrementAttachmentCount().
In `@app/src/org/commcare/views/widgets/MediaWidget.java`:
- Around line 277-316: The attachment limit check in setBinaryData currently
runs before deleting existing media, so replacements are blocked when at the
limit; change the logic in setBinaryData (method) to allow replacements by
either moving the canAddAttachment() check to after the existing-media deletion
block (deleteMedia()) or by making the check conditional: call
((FormEntryActivity)getContext()).canAddAttachment() only when mBinaryName is
null (i.e., when adding new media), otherwise skip the limit check so
replacements proceed, and keep the rest of the flow (createFilePath,
size/extension checks, encryptRecordedFileToDestination, updating
mTempBinaryPath/mBinaryName and incrementAttachmentCount()) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e8238cb-3094-44e8-947c-9146de1da80e
📒 Files selected for processing (4)
app/res/values/strings.xmlapp/src/org/commcare/activities/FormEntryActivity.javaapp/src/org/commcare/views/widgets/ImageWidget.javaapp/src/org/commcare/views/widgets/MediaWidget.java
Check the per-form attachment count before accepting new files in MediaWidget and ImageWidget. Covers camera captures, audio recordings, and file selections. Shows an error when the limit is reached. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
avazirna
force-pushed
the
limit-number-of-form-attachments
branch
from
0d2b245 to
c8531c7
Compare
avazirna
force-pushed
the
limit-number-of-form-attachments
branch
from
c8531c7 to
baf3b9d
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/views/widgets/MediaWidget.java (1)
317-323:⚠️ Potential issue | 🟠 MajorOnly increment attachment count after successful file persistence.
setMediaName(...)at Line 322 increments the global counter even whennewMedia.exists()is false. This can create phantom attachments and block users early.💡 Proposed fix
encryptRecordedFileToDestination(binaryPath); File newMedia = new File(destMediaPath); -if (newMedia.exists()) { - showToast("form.attachment.success"); -} +if (!newMedia.exists()) { + showToast("form.attachment.copy.fail"); + return; +} +showToast("form.attachment.success"); mTempBinaryPath = binaryPath; setMediaName(removeAESExtension(newMedia.getName()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/views/widgets/MediaWidget.java` around lines 317 - 323, The code currently calls setMediaName(removeAESExtension(newMedia.getName())) and assigns mTempBinaryPath even when the file save failed (newMedia.exists() is false), causing the global attachment counter to increment incorrectly; fix this by only setting mTempBinaryPath and calling setMediaName(...) after verifying the file was persisted (i.e., inside the newMedia.exists() true block) so that the attachment count increment in setMediaName only happens on successful saves (refer to MediaWidget class and the setMediaName/removeAESExtension/newMedia.exists() usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/res/values/strings.xml`:
- Line 707: Update the string resource named form_attachment_limit_reached to
use a positional integer placeholder (%1$d) instead of %s, and change its
callers in FormEntryActivity (where this string is formatted) to pass the
attachment count as an int (not a pre-stringified value) into
getString(R.string.form_attachment_limit_reached, count) or equivalent
formatting call so translators can reorder/format the number safely.
In `@app/src/org/commcare/utils/FileUtil.java`:
- Around line 997-1017: The countNumberOfMediaFilesInDirectory method currently
returns primitive int and yields 0 on any exception (hiding failures); change
its signature to return Integer (nullable) and update implementation so it
returns the actual count on success and returns null when an exception occurs
(still log the exception with Logger.exception); ensure all return paths return
an Integer (e.g., Integer.valueOf(count)) and update callers (e.g.,
FormEntryActivity.updateFormAttachmentCount) to only overwrite attachmentCount
when the returned Integer is non-null as suggested.
In `@app/src/org/commcare/views/widgets/MediaWidget.java`:
- Around line 215-232: The current clearMediaData method always calls
decrementAttachmentCount, which can decrement when no media exists (mBinaryName
is null) because clearAnswer always calls clearMediaData; update clearMediaData
to guard the decrement by checking whether media is present before calling
decrementAttachmentCount (e.g., only call decrementAttachmentCount when
mBinaryName != null or when deleteMediaFiles actually removed files), leaving
deleteMediaFiles(mInstanceFolder, mBinaryName), mBinaryName = null, and
mTempBinaryPath = null behavior unchanged otherwise so attachment counts remain
accurate.
In `@app/src/org/commcare/views/widgets/SignatureWidget.java`:
- Around line 189-201: clearAnswer() currently calls clearMediaData() and clears
the bitmap but leaves mImageView and its click listener active, allowing
launchSignatureActivity(...) to be invoked bypassing isAttachmentLimitReached();
update clearAnswer() (or clearMediaData()) to also reset or disable mImageView
and/or remove its click listener (e.g., setImageDrawable(null) and
setOnClickListener(null) or setEnabled(false)) and ensure any path that
re-attaches the preview reinstalls a click handler that checks
isAttachmentLimitReached() before calling launchSignatureActivity(...); touch
the methods clearAnswer(), clearMediaData(), and any code that registers the
preview click listener to implement this change.
---
Outside diff comments:
In `@app/src/org/commcare/views/widgets/MediaWidget.java`:
- Around line 317-323: The code currently calls
setMediaName(removeAESExtension(newMedia.getName())) and assigns mTempBinaryPath
even when the file save failed (newMedia.exists() is false), causing the global
attachment counter to increment incorrectly; fix this by only setting
mTempBinaryPath and calling setMediaName(...) after verifying the file was
persisted (i.e., inside the newMedia.exists() true block) so that the attachment
count increment in setMediaName only happens on successful saves (refer to
MediaWidget class and the setMediaName/removeAESExtension/newMedia.exists()
usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36406409-3803-43c9-967b-cbd5633ed424
📒 Files selected for processing (10)
app/res/values/strings.xmlapp/src/org/commcare/activities/FormEntryActivity.javaapp/src/org/commcare/utils/FileUtil.javaapp/src/org/commcare/views/widgets/AudioWidget.javaapp/src/org/commcare/views/widgets/DocumentWidget.javaapp/src/org/commcare/views/widgets/ImageWidget.javaapp/src/org/commcare/views/widgets/MediaWidget.javaapp/src/org/commcare/views/widgets/QuestionWidget.javaapp/src/org/commcare/views/widgets/SignatureWidget.javaapp/src/org/commcare/views/widgets/VideoWidget.java
| <string name="network_notification_service_running">Running network requests...</string> | ||
| <string name="network_notification_service_downloading_files">Downloading application files...</string> | ||
| <string name="form_attachment_limit_reached_title">Attachment limit reached</string> | ||
| <string name="form_attachment_limit_reached">This form has reached the maximum of %s attachments. To add a new one, remove an existing attachment first.</string> |
Contributor
There was a problem hiding this comment.
Use a positional numeric placeholder here.
%s works in English, but it makes localization harder and forces the caller to stringify the number first. Prefer %1$d for this count so translators can reorder it safely.
🛠️ Suggested change
- <string name="form_attachment_limit_reached">This form has reached the maximum of %s attachments. To add a new one, remove an existing attachment first.</string>
+ <string name="form_attachment_limit_reached">This form has reached the maximum of %1$d attachments. To add a new one, remove an existing attachment first.</string>And update the call site in app/src/org/commcare/activities/FormEntryActivity.java:
- String msg = StringUtils.getStringRobust(this, R.string.form_attachment_limit_reached,
- String.valueOf(MAX_ATTACHMENTS));
+ String msg = StringUtils.getStringRobust(this, R.string.form_attachment_limit_reached,
+ MAX_ATTACHMENTS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/res/values/strings.xml` at line 707, Update the string resource named
form_attachment_limit_reached to use a positional integer placeholder (%1$d)
instead of %s, and change its callers in FormEntryActivity (where this string is
formatted) to pass the attachment count as an int (not a pre-stringified value)
into getString(R.string.form_attachment_limit_reached, count) or equivalent
formatting call so translators can reorder/format the number safely.
Comment on lines
+997
to
+1017
| public static int countNumberOfMediaFilesInDirectory(String dirPath) { | ||
| int count = 0; | ||
| try { | ||
| File dir = new File(dirPath); | ||
| if (dir.exists() && dir.isDirectory()) { | ||
| File[] files = dir.listFiles(); | ||
| if (files != null) { | ||
| for (File file : files) { | ||
| if (file.isFile() && | ||
| !file.getName().endsWith(XML_EXTENSION) && | ||
| (FormUploadUtil.isSupportedMultimediaFile(file.getName()) || | ||
| file.getName().endsWith(MediaWidget.AES_EXTENSION))) { | ||
| count++; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| // log non-fatal exception and return count as 0 | ||
| Logger.exception("Error counting media files: ", e); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't fail open when attachment recount throws.
This method returns 0 for any exception, and FormEntryActivity.updateFormAttachmentCount() then trusts that value. A bad instance path or listFiles() failure will silently undercount existing attachments and let the user exceed the cap for the rest of the session.
🛠️ Suggested direction
-public static int countNumberOfMediaFilesInDirectory(String dirPath) {
+public static Integer countNumberOfMediaFilesInDirectory(String dirPath) {
int count = 0;
try {
File dir = new File(dirPath);
if (dir.exists() && dir.isDirectory()) {
File[] files = dir.listFiles();
@@
}
} catch (Exception e) {
- // log non-fatal exception and return count as 0
Logger.exception("Error counting media files: ", e);
+ return null;
}
return count;
}Then only overwrite attachmentCount when recount succeeds:
Integer counted = FileUtil.countNumberOfMediaFilesInDirectory(FormEntryInstanceState.getInstanceFolder());
if (counted != null) {
attachmentCount = counted;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/utils/FileUtil.java` around lines 997 - 1017, The
countNumberOfMediaFilesInDirectory method currently returns primitive int and
yields 0 on any exception (hiding failures); change its signature to return
Integer (nullable) and update implementation so it returns the actual count on
success and returns null when an exception occurs (still log the exception with
Logger.exception); ensure all return paths return an Integer (e.g.,
Integer.valueOf(count)) and update callers (e.g.,
FormEntryActivity.updateFormAttachmentCount) to only overwrite attachmentCount
when the returned Integer is non-null as suggested.
Comment on lines
+215
to
232
| clearMediaData(); | ||
| togglePlayButton(false); | ||
| } | ||
|
|
||
| private void deleteMedia() { | ||
|
|
||
| @Override | ||
| public void setMediaName(String mediaName) { | ||
| mBinaryName = mediaName; | ||
| incrementAttachmentCount(); | ||
| } | ||
|
|
||
| @Override | ||
| public void clearMediaData() { | ||
| deleteMediaFiles(mInstanceFolder, mBinaryName); | ||
| mBinaryName = null; | ||
| mTempBinaryPath = null; | ||
| decrementAttachmentCount(); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard decrement to only run when media actually exists.
At Line 231, decrementAttachmentCount() runs even when no media is attached. Combined with Line 215 (clearAnswer() always calling clearMediaData()), this can undercount attachments and weaken limit enforcement.
💡 Proposed fix
`@Override`
public void clearMediaData() {
- deleteMediaFiles(mInstanceFolder, mBinaryName);
- mBinaryName = null;
- mTempBinaryPath = null;
- decrementAttachmentCount();
+ if (mBinaryName != null) {
+ deleteMediaFiles(mInstanceFolder, mBinaryName);
+ mBinaryName = null;
+ decrementAttachmentCount();
+ }
+ mTempBinaryPath = 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.
Suggested change
| clearMediaData(); | |
| togglePlayButton(false); | |
| } | |
| private void deleteMedia() { | |
| @Override | |
| public void setMediaName(String mediaName) { | |
| mBinaryName = mediaName; | |
| incrementAttachmentCount(); | |
| } | |
| @Override | |
| public void clearMediaData() { | |
| deleteMediaFiles(mInstanceFolder, mBinaryName); | |
| mBinaryName = null; | |
| mTempBinaryPath = null; | |
| decrementAttachmentCount(); | |
| } | |
| `@Override` | |
| public void clearMediaData() { | |
| if (mBinaryName != null) { | |
| deleteMediaFiles(mInstanceFolder, mBinaryName); | |
| mBinaryName = null; | |
| decrementAttachmentCount(); | |
| } | |
| mTempBinaryPath = null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/views/widgets/MediaWidget.java` around lines 215 - 232,
The current clearMediaData method always calls decrementAttachmentCount, which
can decrement when no media exists (mBinaryName is null) because clearAnswer
always calls clearMediaData; update clearMediaData to guard the decrement by
checking whether media is present before calling decrementAttachmentCount (e.g.,
only call decrementAttachmentCount when mBinaryName != null or when
deleteMediaFiles actually removed files), leaving
deleteMediaFiles(mInstanceFolder, mBinaryName), mBinaryName = null, and
mTempBinaryPath = null behavior unchanged otherwise so attachment counts remain
accurate.
Comment on lines
+189
to
+201
| @Override | ||
| public void clearMediaData() { | ||
| MediaWidget.deleteMediaFiles(mInstanceFolder, mBinaryName); | ||
| // clean up variables | ||
| mBinaryName = null; | ||
| decrementAttachmentCount(); | ||
| } | ||
|
|
||
|
|
||
| @Override | ||
| public void clearAnswer() { | ||
| // remove the file | ||
| deleteMedia(); | ||
| clearMediaData(); |
Contributor
There was a problem hiding this comment.
Clearing the signature leaves a clickable path that skips the new limit check.
clearAnswer() clears the bitmap, but it does not remove or disable mImageView. The existing preview click listener still calls launchSignatureActivity(...) directly, so after clearing and later hitting the cap again, tapping the blank preview can still start signature capture without going through isAttachmentLimitReached().
🛠️ Suggested change
`@Override`
public void clearMediaData() {
MediaWidget.deleteMediaFiles(mInstanceFolder, mBinaryName);
// clean up variables
mBinaryName = null;
+ if (mImageView != null) {
+ mImageView.setImageBitmap(null);
+ mImageView.setOnClickListener(null);
+ removeView(mImageView);
+ mImageView = null;
+ }
decrementAttachmentCount();
}
`@Override`
public void clearAnswer() {
// remove the file
clearMediaData();
- mImageView.setImageBitmap(null);
mErrorTextView.setVisibility(View.GONE);
// reset buttons
mSignButton.setText(getContext().getString(R.string.sign_button));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/views/widgets/SignatureWidget.java` around lines 189 -
201, clearAnswer() currently calls clearMediaData() and clears the bitmap but
leaves mImageView and its click listener active, allowing
launchSignatureActivity(...) to be invoked bypassing isAttachmentLimitReached();
update clearAnswer() (or clearMediaData()) to also reset or disable mImageView
and/or remove its click listener (e.g., setImageDrawable(null) and
setOnClickListener(null) or setEnabled(false)) and ensure any path that
re-attaches the preview reinstalls a click handler that checks
isAttachmentLimitReached() before calling launchSignatureActivity(...); touch
the methods clearAnswer(), clearMediaData(), and any code that registers the
preview click listener to implement this change.
| <string name="network_notification_service_running">Running network requests...</string> | ||
| <string name="network_notification_service_downloading_files">Downloading application files...</string> | ||
| <string name="form_attachment_limit_reached_title">Attachment limit reached</string> | ||
| <string name="form_attachment_limit_reached">This form has reached the maximum of %s attachments. To add a new one, remove an existing attachment first.</string> |
Contributor
There was a problem hiding this comment.
Should use %d here
Comment on lines
+450
to
+457
| public void showFormAttachmentLimitReachedError() { | ||
| String title = StringUtils.getStringRobust(this, R.string.form_attachment_limit_reached_title); | ||
| String msg = StringUtils.getStringRobust(this, R.string.form_attachment_limit_reached, | ||
| String.valueOf(MAX_ATTACHMENTS)); | ||
| CommCareAlertDialog dialog = StandardAlertDialog.getBasicAlertDialog( | ||
| title, msg, (dialog1, which) -> dialog1.dismiss()); | ||
| showAlertDialog(dialog); | ||
| } |
Contributor
There was a problem hiding this comment.
Can we add a quick screenshot of what this UI looks like to the PR description
| } | ||
| } | ||
| } catch (Exception e) { | ||
| // log non-fatal exception and return count as 0 |
Contributor
There was a problem hiding this comment.
Is there a specific reason for doing it this way vs returning null for the count?
Comment on lines
+233
to
+237
| @Override | ||
| public void setMediaName(String mediaName) { | ||
| mBinaryName = mediaName; | ||
| incrementAttachmentCount(); | ||
| } |
Contributor
There was a problem hiding this comment.
This setter function should follow the single responsibility principle and only be responsible for setting the media name. Is there a better place where we can call incrementAttachmentCount()?
| @Override | ||
| public void setMediaName(String mediaName) { | ||
| mBinaryName = mediaName; | ||
| incrementAttachmentCount(); |
Contributor
There was a problem hiding this comment.
Similar concern here
Comment on lines
+236
to
+244
| protected boolean isAttachmentLimitReached() { | ||
| if (getContext() instanceof FormEntryActivity activity && this instanceof MediaCapableWidget mediaCapableWidget) { | ||
| if (mediaCapableWidget.getMediaName() == null && !activity.canAddAttachment()) { | ||
| activity.showFormAttachmentLimitReachedError(); | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
On first glance this function seems to only be responsible for checking if the limit is reached, but in reality it is also showing the error message. Maybe this simplest fix is to rename this function?
Suggested change
| protected boolean isAttachmentLimitReached() { | |
| if (getContext() instanceof FormEntryActivity activity && this instanceof MediaCapableWidget mediaCapableWidget) { | |
| if (mediaCapableWidget.getMediaName() == null && !activity.canAddAttachment()) { | |
| activity.showFormAttachmentLimitReachedError(); | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| protected boolean showAttachmentErrorIfLimitReached() { | |
| if (getContext() instanceof FormEntryActivity activity && this instanceof MediaCapableWidget mediaCapableWidget) { | |
| if (mediaCapableWidget.getMediaName() == null && !activity.canAddAttachment()) { | |
| activity.showFormAttachmentLimitReachedError(); | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
| @Override | ||
| public void setMediaName(String mediaName) { | ||
| mBinaryName = mediaName; | ||
| incrementAttachmentCount(); |
Contributor
There was a problem hiding this comment.
Same concern here as mentioned above
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Product Description
Adds a per-form attachment limit (currently set to 50). When a user tries to add a new attachment beyond the limit — via camera, audio recording, file picker, or any media widget — they see an error message asking them to delete an existing attachment first.
Technical Summary
attachmentCounttracker andMAX_ATTACHMENTSconstant inFormEntryActivitycanAddAttachment(),incrementAttachmentCount(), anddecrementAttachmentCount()methods toFormEntryActivityMediaCapableWidgetinterface inQuestionWidgetwithgetMediaName(),setMediaName(), andclearMediaData()methods to standardize media attachment tracking across widget typesFormEntryActivitymaintains an in-memory attachment count, initialized from the instance folder on form load viaFileUtil.countNumberOfMediaFilesInDirectory()Audio,Video,Document,Image,SignatureSafety Assurance
Safety story
isAttachmentLimitReached()checksgetMediaName()== nullAutomated test coverage
Unit tests needed for
canAddAttachment()boundary conditions and widget rejection behavior.QA Plan
Labels and Review