Fix IsResolved is not set to false despite associated Loan Return Preparations being deleted or unresolved#8077
Open
Iwantexpresso wants to merge 16 commits into
Open
Fix IsResolved is not set to false despite associated Loan Return Preparations being deleted or unresolved#8077Iwantexpresso wants to merge 16 commits into
Iwantexpresso wants to merge 16 commits into
Conversation
Triggered by c89c449 on branch refs/heads/issue-4883
…tring instead of ints
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds API logic to compute Loanpreparation.extra["isresolved"] from integerized quantities and casts posted quantity fields to int when summing loan return preparations; applies widespread non-functional frontend formatting and a localization key quoting change. ChangesLoan Preparation IsResolved Resolution
Code Formatting and Localization Alignment
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Iwantexpresso
changed the title
Triggered by c3b13bf on branch refs/heads/issue-4883
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
specifyweb/frontend/js_src/lib/localization/utils/config.ts (1)
27-27: 💤 Low valueInconsistent quote style for object key.
All other language code keys in
languageCodeMapper(lines 19–26) use quoted string format ('en-us','ru-ru', etc.), butnbnow uses an unquoted identifier. For consistency, consider using the same quote style as the other keys.Suggested fix for consistency
- nb: 'nb_NO', + 'nb': 'nb_NO',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/localization/utils/config.ts` at line 27, The languageCodeMapper object contains an inconsistent key style: the nb key is unquoted while all other keys (e.g., 'en-us', 'ru-ru') are quoted; update the nb entry in languageCodeMapper to use the same quoted string format ('nb') to match the surrounding keys and maintain consistent style across the object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/backend/businessrules/rules/Loanprep_rules.py`:
- Around line 33-35: The registration code currently swallows failures and only
logs them; change those paths so that when a signal lookup yields None (e.g.,
the check using the variable signal and signal_name) you raise a ValueError (or
re-raise the original exception) instead of continuing, and for other unexpected
registration exceptions catch them only to log context then re-raise; also
replace silent missing-model logs with at least logger.warning or raise a clear
error so misconfiguration is visible. Locate the three registration blocks
around the signal/signal_name checks (the branch that currently logs when signal
is None and the two similar blocks later) and update them to raise on missing
signals, and to re-raise any unexpected exceptions after logging contextual
information.
In `@specifyweb/backend/interactions/cog_preps.py`:
- Around line 550-553: The sum comprehensions for total_quantity_returned and
total_quantity_resolved use raw int(...) which will raise on None/empty strings;
update the list comprehensions in cog_preps.py (the expressions that compute
total_quantity_returned and total_quantity_resolved) to coerce each item safely,
e.g. use int(loan_return.get("quantityreturned") or 0) and
int(loan_return.get("quantityresolved") or 0) (or int((loan_return["..."] if
loan_return.get("...") else 0))) so empty/nullable payload fields become 0
instead of throwing.
In `@specifyweb/specify/api/calculated_fields.py`:
- Around line 118-121: Coerce incoming quantity fields to integers before
comparing: convert the values assigned to quantity_resolved and total_quantity
(from data.get('quantityresolved') and data.get('quantity')) to int with safe
defaults (e.g., 0) to avoid string comparisons or type errors, then compute
is_resolved = (quantity_resolved >= total_quantity) and set extra['isresolved']
accordingly in calculated_fields.py so the variables quantity_resolved,
total_quantity, is_resolved and extra['isresolved'] are always based on integer
values.
---
Nitpick comments:
In `@specifyweb/frontend/js_src/lib/localization/utils/config.ts`:
- Line 27: The languageCodeMapper object contains an inconsistent key style: the
nb key is unquoted while all other keys (e.g., 'en-us', 'ru-ru') are quoted;
update the nb entry in languageCodeMapper to use the same quoted string format
('nb') to match the surrounding keys and maintain consistent style across the
object.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f9558d1-52c8-4bf5-a74d-1db0de260cb5
📒 Files selected for processing (16)
specifyweb/backend/businessrules/rules/Loanprep_rules.pyspecifyweb/backend/interactions/cog_preps.pyspecifyweb/frontend/js_src/lib/components/ExpressSearchConfig/ExpressSearchConfigDialog.tsxspecifyweb/frontend/js_src/lib/components/ExpressSearchConfig/ResultsOrderingTab.tsxspecifyweb/frontend/js_src/lib/components/ExpressSearchConfig/__tests__/ExpressSearchConfigEditor.test.tsxspecifyweb/frontend/js_src/lib/components/ExpressSearchConfig/__tests__/RelatedTablesTab.test.tsxspecifyweb/frontend/js_src/lib/components/ExpressSearchConfig/__tests__/ResultsOrderingTab.test.tsxspecifyweb/frontend/js_src/lib/components/FormCells/FormTable.tsxspecifyweb/frontend/js_src/lib/components/Header/ExpressSearchHooks.tsxspecifyweb/frontend/js_src/lib/components/Header/ExpressSearchTask.tsxspecifyweb/frontend/js_src/lib/components/Notifications/NotificationRenderers.tsxspecifyweb/frontend/js_src/lib/components/WbPlanView/navigator.tsspecifyweb/frontend/js_src/lib/localization/common.tsspecifyweb/frontend/js_src/lib/localization/utils/config.tsspecifyweb/frontend/js_src/lib/utils/schemaVisibility.tsspecifyweb/specify/api/calculated_fields.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
There was a problem hiding this comment.
♻️ Duplicate comments (1)
specifyweb/backend/interactions/cog_preps.py (1)
550-553:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEdge case handling remains incomplete despite Firefox fix.
While the
int()casting resolves the Firefox string-to-int issue, the code still doesn't guard againstloan_returnbeingNoneor the dictionary keys being absent—both would raise exceptions and break the update flow.Recommended fix (from previous review)
total_quantity_returned = sum( - [int(loan_return["quantityreturned"] or 0) for loan_return in loan_return_data]) + int((loan_return or {}).get("quantityreturned") or 0) + for loan_return in loan_return_data + ) total_quantity_resolved = sum( - [int(loan_return["quantityresolved"] or 0) for loan_return in loan_return_data]) + int((loan_return or {}).get("quantityresolved") or 0) + for loan_return in loan_return_data + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/interactions/cog_preps.py` around lines 550 - 553, The summation lines for total_quantity_returned and total_quantity_resolved assume every loan_return in loan_return_data is a dict with the keys "quantityreturned" and "quantityresolved" and that values are numeric/parseable; update them to defensively handle None entries and missing keys by skipping None loan_return objects and using dict.get with a 0 default, then safely coerce the value to int (fallback to 0 on empty/invalid strings) before summing. Modify the comprehensions that reference loan_return_data / loan_return to first check loan_return is truthy and use loan_return.get("quantityreturned", 0) and loan_return.get("quantityresolved", 0) with a safe int conversion fallback for both total_quantity_returned and total_quantity_resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@specifyweb/backend/interactions/cog_preps.py`:
- Around line 550-553: The summation lines for total_quantity_returned and
total_quantity_resolved assume every loan_return in loan_return_data is a dict
with the keys "quantityreturned" and "quantityresolved" and that values are
numeric/parseable; update them to defensively handle None entries and missing
keys by skipping None loan_return objects and using dict.get with a 0 default,
then safely coerce the value to int (fallback to 0 on empty/invalid strings)
before summing. Modify the comprehensions that reference loan_return_data /
loan_return to first check loan_return is truthy and use
loan_return.get("quantityreturned", 0) and loan_return.get("quantityresolved",
0) with a safe int conversion fallback for both total_quantity_returned and
total_quantity_resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b0c6a52-d62b-4877-8b8d-dc97131d1e64
📒 Files selected for processing (1)
specifyweb/backend/interactions/cog_preps.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
specifyweb/specify/api/calculated_fields.py (1)
118-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden integer parsing to avoid runtime errors on empty/null payloads.
int(data.get(...))can raise before fallback is applied (e.g.,None,''), which can still break the Firefox/manual-create path and skipisresolvedcalculation.Suggested minimal fix
- quantity_resolved = int(data.get('quantityresolved')) or 0 - total_quantity = int(data.get('quantity')) or 0 + quantity_resolved = int(data.get('quantityresolved') or 0) + total_quantity = int(data.get('quantity') or 0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/api/calculated_fields.py` around lines 118 - 119, The integer parsing for quantity_resolved and total_quantity is unsafe: int(data.get(...)) can raise on None or empty string and prevent downstream isresolved logic; update the parsing in specify/api/calculated_fields.py so you first retrieve the raw values (e.g., raw_quantity_resolved = data.get('quantityresolved') and raw_total_quantity = data.get('quantity')), then convert using a safe pattern (treat None/'' as 0) before calling int (or wrap int(...) in a small try/except that falls back to 0). Ensure you replace the current assignments to quantity_resolved and total_quantity with this safe parsing approach and keep the rest of isresolved calculation intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@specifyweb/specify/api/calculated_fields.py`:
- Around line 118-119: The integer parsing for quantity_resolved and
total_quantity is unsafe: int(data.get(...)) can raise on None or empty string
and prevent downstream isresolved logic; update the parsing in
specify/api/calculated_fields.py so you first retrieve the raw values (e.g.,
raw_quantity_resolved = data.get('quantityresolved') and raw_total_quantity =
data.get('quantity')), then convert using a safe pattern (treat None/'' as 0)
before calling int (or wrap int(...) in a small try/except that falls back to
0). Ensure you replace the current assignments to quantity_resolved and
total_quantity with this safe parsing approach and keep the rest of isresolved
calculation intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8120b7f-7f7d-43e3-bfa1-206f184cd121
📒 Files selected for processing (1)
specifyweb/specify/api/calculated_fields.py
Iwantexpresso
requested review from
a team and
Copilot
and removed request for
Copilot
bhumikaguptaa
approved these changes
May 21, 2026
Collaborator
bhumikaguptaa
left a comment
bhumikaguptaa
left a comment
There was a problem hiding this comment.
For Firefox Specifically ( #8076) :
- Make sure there is no longer an error related to the manual creation of LoanReturnPreps
- (optional) test these on a different browser to ensure functionality stayed the same between them
For any browser (#4883):
- make sure the isResolved box is checked
- Make sure the isResolved box is unchecked
- IsResolved checkbox remains unchecked until the amount of loan return preps is equivalent to the amount of loan preparations available
Everything works perfectly. I did not run into any error messages. I also tested it on Safari.
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.
Fixes #4883 and #8076
Previously there was an inconsistency between the isResolved checkbox not being properly set to false when deleting loan return preps. during debugging for this issue, there was a new issue found specific to Firefox browsers that occurred while manually creating loan return preps due to how it handled JSON field parsing. both are addressed within this pr. and both features should be working as intended.
Checklist
self-explanatory (or properly documented)
Testing instructions
For Firefox Specifically ( #8076) :
For any browser (#4883):
Summary by CodeRabbit
Bug Fixes
New Features
Refactor