OSIDB-4985: Fix revert issues.#1276
Conversation
|
Warning Review limit reached
More reviews will be available in 37 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Flaw refreshes in signal handlers before saving persisted state, and a new pytest module with pghistory helpers plus two regression tests that verify saving related Affect/Tracker objects does not revert an updated Flaw.statement. ChangesSignal fix and regression tests for Flaw.statement
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![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)
osidb/tests/test_flaw_revert_bug.py (1)
77-134: 💤 Low valueConsider reducing debug print statements.
The test contains extensive print statements (lines 77, 91, 104, 109, 113, 119-121, 126-130, 132-133) that will clutter test output on every run. While helpful during initial bug reproduction, these should be removed, reduced, or gated behind a debug flag for the production test suite to improve readability and maintainability.
🤖 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 `@osidb/tests/test_flaw_revert_bug.py` around lines 77 - 134, This test function is noisy due to many print() calls; remove or convert them to conditional/debug logging to avoid cluttering test output. Replace the print(...) calls around the creation/update/verification steps (the prints that reference flaw, affects, new_statement, and the final audit/print_audit) with either logger.debug(...) (using a module-level logger) or drop them entirely, or gate them behind a pytest --debug/verbose fixture flag so normal test runs stay quiet; ensure imports/usage reference the existing flaw, affects, get_flaw_audit, and print_audit symbols so behavior and assertions remain unchanged.
🤖 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 `@osidb/tests/test_flaw_revert_bug.py`:
- Around line 186-189: The assertion in test_flaw_revert_bug.py that checks
"assert flaw.statement == new_statement" has a misleading message saying "BUG
REPRODUCED"; update the assertion message to clearly state this is a test
failure indicating an unexpected reversion (e.g., "UNEXPECTED: Statement
reverted to old value when affects were saved"), so modify the string attached
to the assert referencing flaw.statement and new_statement to reflect an
unexpected failure rather than successful reproduction of a bug.
- Around line 28-36: The helper arrange_event_diffs accesses
s.pgh_data['task_key'] directly which can raise KeyError; update
arrange_event_diffs to safely retrieve the task key (e.g., using dict.get with a
fallback like "<no-task-key>" or conditional logic) before formatting the text
so missing keys don't break test printing, and ensure you reference s.pgh_data
and the arrange_event_diffs function when making the change.
- Around line 136-140: The assertion message wrongly says "BUG REPRODUCED" which
is misleading; update the assertion in the test that checks flaw.statement ==
new_statement to a failure message that reports an unexpected reversion (e.g.,
"Unexpected reversion of Flaw.statement"), and include both expected
(new_statement) and actual (flaw.statement) values for context so the test
clearly signals a failing fix rather than a successful reproduction; modify the
message around the assert that references flaw and new_statement accordingly.
---
Nitpick comments:
In `@osidb/tests/test_flaw_revert_bug.py`:
- Around line 77-134: This test function is noisy due to many print() calls;
remove or convert them to conditional/debug logging to avoid cluttering test
output. Replace the print(...) calls around the creation/update/verification
steps (the prints that reference flaw, affects, new_statement, and the final
audit/print_audit) with either logger.debug(...) (using a module-level logger)
or drop them entirely, or gate them behind a pytest --debug/verbose fixture flag
so normal test runs stay quiet; ensure imports/usage reference the existing
flaw, affects, get_flaw_audit, and print_audit symbols so behavior and
assertions remain 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 98fbe4eb-9d80-4d94-8256-fb77e3e2c559
📒 Files selected for processing (1)
osidb/tests/test_flaw_revert_bug.py
| def arrange_event_diffs(flaw_story): | ||
| data_list = [] | ||
| for s in flaw_story: | ||
| text = f"************{s.pgh_data['task_key']} - {s.pgh_created_at}******************" | ||
| text += f"\n{s.pgh_context}" | ||
| text += f"\n{s.pgh_diff}" | ||
| text += "**************************************************" | ||
| data_list.append(text) | ||
| return data_list |
There was a problem hiding this comment.
Add safety check for dictionary access.
Line 31 accesses s.pgh_data['task_key'] without checking if the key exists, which could raise a KeyError and cause test infrastructure failures during audit printing.
🛡️ Proposed fix
def arrange_event_diffs(flaw_story):
data_list = []
for s in flaw_story:
- text = f"************{s.pgh_data['task_key']} - {s.pgh_created_at}******************"
+ task_key = s.pgh_data.get('task_key', 'unknown')
+ text = f"************{task_key} - {s.pgh_created_at}******************"
text += f"\n{s.pgh_context}"
text += f"\n{s.pgh_diff}"
text += "**************************************************"
data_list.append(text)
return data_list📝 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.
| def arrange_event_diffs(flaw_story): | |
| data_list = [] | |
| for s in flaw_story: | |
| text = f"************{s.pgh_data['task_key']} - {s.pgh_created_at}******************" | |
| text += f"\n{s.pgh_context}" | |
| text += f"\n{s.pgh_diff}" | |
| text += "**************************************************" | |
| data_list.append(text) | |
| return data_list | |
| def arrange_event_diffs(flaw_story): | |
| data_list = [] | |
| for s in flaw_story: | |
| task_key = s.pgh_data.get('task_key', 'unknown') | |
| text = f"************{task_key} - {s.pgh_created_at}******************" | |
| text += f"\n{s.pgh_context}" | |
| text += f"\n{s.pgh_diff}" | |
| text += "**************************************************" | |
| data_list.append(text) | |
| return data_list |
🤖 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 `@osidb/tests/test_flaw_revert_bug.py` around lines 28 - 36, The helper
arrange_event_diffs accesses s.pgh_data['task_key'] directly which can raise
KeyError; update arrange_event_diffs to safely retrieve the task key (e.g.,
using dict.get with a fallback like "<no-task-key>" or conditional logic) before
formatting the text so missing keys don't break test printing, and ensure you
reference s.pgh_data and the arrange_event_diffs function when making the
change.
There was a problem hiding this comment.
This is just for a test to see a more comfortable history .
There was a problem hiding this comment.
I will remove this soon
There was a problem hiding this comment.
@roduran-dev, understood — since it's purely a diagnostic helper for visualizing history, a KeyError there would only affect test output readability rather than test correctness. Fair enough, no changes needed.
(◕‿◕✿)
roduran-dev
force-pushed
the
OSIDB-4985-put-flaw-causes-a-revert
branch
from
65fe829 to
ecfa1a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@osidb/signals.py`:
- Around line 129-130: The handler updated_local_updated_dt_affectcvss still
calls instance.affect.flaw.save(...) on a potentially stale relation; update the
signal to refresh the related Flaw before saving by calling refresh_from_db() on
the related object (e.g. instance.affect.flaw.refresh_from_db()) and then call
save(auto_timestamps=False, raise_validation_error=False) to match the other
fixed path and avoid stale-state reversion.
In `@osidb/tests/test_flaw_revert_bug.py`:
- Around line 149-151: The final assertion in
test_tracker_does_not_revert_statement uses the stale in-memory Flaw instance
`flaw`, so refresh the fixture from the DB before asserting; after the
tracker/save operation call `flaw.refresh_from_db()` (or the ORM equivalent) and
then assert `flaw.statement == new_statement` to ensure you are validating
persisted state rather than the old in-memory 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: Enterprise
Run ID: dd5307a0-232d-47b8-af0d-66f532c7b7f6
📒 Files selected for processing (2)
osidb/signals.pyosidb/tests/test_flaw_revert_bug.py
roduran-dev
force-pushed
the
OSIDB-4985-put-flaw-causes-a-revert
branch
4 times, most recently
from
c765d34 to
d4e0444
Compare
roduran-dev
changed the title
roduran-dev
force-pushed
the
OSIDB-4985-put-flaw-causes-a-revert
branch
4 times, most recently
from
ec95a60 to
46b6fc3
Compare
roduran-dev
force-pushed
the
OSIDB-4985-put-flaw-causes-a-revert
branch
from
46b6fc3 to
f8eac93
Compare
roduran-dev
requested review from
JakubFrejlach,
Jincxz,
MrMarble and
costaconrado
2 participants
Fix the revert Flaw fields issue by refreshing the Flaw instance before re saving the flaw.
The rever can happen when someone updates a tracker for a flaw and then saves the Flaw while the tracker process is happening with a stale version of the flaw..
User Adds/updates The tracker TA of Flaw F1 on any of this API endpoints.
The change triggers an update on Jira TA because TRACKERS_SYNC_TO_JIRA is true.
Jira is updated (and updates its last changed date for TA)
User runs updates on flaw F1 (To change FA to FB)
a minute later the JiraTrackerCollector process TA because it has a newer date
At some point (maybe here) it gets flaw F1 with FA
User from point 4 finish the process saving the F1 with FB
Process from point 6 runs a save F1 with FA causing the revert.
Possible Catalysts:
Attachments & Links:
Logs digested that show the overriding case
Jira ticket