OLS-1946: Change double negative in user data collection config #271
Conversation
WalkthroughThis change refactors all configuration, code, documentation, and tests to replace the negatively-named boolean flags Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Config
participant DataCollector
Client->>API: Request config/info/feedback
API->>Config: Read user_data_collection
Config-->>API: feedback_enabled, transcripts_enabled
API->>DataCollector: Collect data if enabled
DataCollector->>Config: Check feedback_enabled/transcripts_enabled
DataCollector-->>API: Proceed or skip based on flags
API-->>Client: Respond based on enabled features
Estimated code review effort4 (~80 minutes) Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
docs/openapi.json (2)
1524-1528: Wrong title & default value – semantics invertedThe property has been renamed to
feedback_enabled, yet:
titlestill says “Feedback Disabled”.defaultremains true, which now means “enabled by default” instead of the previous behaviour “disabled unless explicitly enabled”.Unless the product requirement changed, this silently flips runtime behaviour.
Apply:
"feedback_enabled": { - "type": "boolean", - "title": "Feedback Disabled", - "default": true + "type": "boolean", + "title": "Feedback Enabled", + "default": false # keep behaviour identical to prior `feedback_disabled: true` }
1540-1544: Same issue fortranscripts_enabledThe
titlestill references “Transcripts Disabled” and thedefaultis set totrue, again reversing the old semantics."transcripts_enabled": { - "type": "boolean", - "title": "Transcripts Disabled", - "default": true + "type": "boolean", + "title": "Transcripts Enabled", + "default": false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
README.md(3 hunks)docs/config.puml(1 hunks)docs/openapi.json(3 hunks)docs/openapi.md(1 hunks)lightspeed-stack.yaml(1 hunks)src/app/endpoints/config.py(1 hunks)src/app/endpoints/feedback.py(1 hunks)src/app/endpoints/query.py(1 hunks)src/models/config.py(1 hunks)src/services/data_collector.py(3 hunks)tests/configuration/lightspeed-stack.yaml(1 hunks)tests/integration/test_configuration.py(1 hunks)tests/unit/__init__.py(1 hunks)tests/unit/app/endpoints/test_config.py(1 hunks)tests/unit/app/endpoints/test_conversations.py(1 hunks)tests/unit/app/endpoints/test_feedback.py(2 hunks)tests/unit/app/endpoints/test_info.py(1 hunks)tests/unit/app/endpoints/test_models.py(3 hunks)tests/unit/app/endpoints/test_query.py(4 hunks)tests/unit/app/endpoints/test_streaming_query.py(1 hunks)tests/unit/models/test_config.py(12 hunks)tests/unit/services/test_data_collector.py(11 hunks)tests/unit/test_configuration.py(9 hunks)tests/unit/utils/test_common.py(6 hunks)
🧬 Code Graph Analysis (8)
src/app/endpoints/feedback.py (1)
src/configuration.py (2)
configuration(47-52)user_data_collection_configuration(71-76)
tests/unit/utils/test_common.py (1)
src/models/config.py (1)
UserDataCollection(118-136)
tests/unit/test_configuration.py (1)
src/configuration.py (1)
user_data_collection_configuration(71-76)
tests/unit/services/test_data_collector.py (1)
src/configuration.py (1)
user_data_collection_configuration(71-76)
src/app/endpoints/query.py (1)
src/configuration.py (2)
configuration(47-52)user_data_collection_configuration(71-76)
tests/unit/app/endpoints/test_feedback.py (2)
src/configuration.py (2)
configuration(47-52)user_data_collection_configuration(71-76)src/app/endpoints/feedback.py (1)
is_feedback_enabled(41-47)
tests/unit/app/endpoints/test_query.py (1)
src/configuration.py (1)
user_data_collection_configuration(71-76)
tests/unit/models/test_config.py (1)
src/models/config.py (1)
UserDataCollection(118-136)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/app/endpoints/feedback.py (1)
src/configuration.py (2)
configuration(47-52)user_data_collection_configuration(71-76)
tests/unit/utils/test_common.py (1)
src/models/config.py (1)
UserDataCollection(118-136)
tests/unit/test_configuration.py (1)
src/configuration.py (1)
user_data_collection_configuration(71-76)
tests/unit/services/test_data_collector.py (1)
src/configuration.py (1)
user_data_collection_configuration(71-76)
src/app/endpoints/query.py (1)
src/configuration.py (2)
configuration(47-52)user_data_collection_configuration(71-76)
tests/unit/app/endpoints/test_feedback.py (2)
src/configuration.py (2)
configuration(47-52)user_data_collection_configuration(71-76)src/app/endpoints/feedback.py (1)
is_feedback_enabled(41-47)
tests/unit/app/endpoints/test_query.py (1)
src/configuration.py (1)
user_data_collection_configuration(71-76)
tests/unit/models/test_config.py (1)
src/models/config.py (1)
UserDataCollection(118-136)
🔇 Additional comments (43)
tests/configuration/lightspeed-stack.yaml (1)
19-19: LGTM! Correct boolean logic inversion.The change from
feedback_disabled: falsetofeedback_enabled: truecorrectly maintains the same functional behavior while eliminating the confusing double negative.tests/integration/test_configuration.py (1)
58-58: LGTM! Test assertion correctly updated.The test assertion properly reflects the new positive naming convention while maintaining the same logical test behavior.
lightspeed-stack.yaml (1)
19-21: LGTM! Both configuration flags correctly refactored.The changes from
feedback_disabled: falsetofeedback_enabled: trueandtranscripts_disabled: falsetotranscripts_enabled: truecorrectly eliminate double negatives while maintaining the same functional behavior.tests/unit/app/endpoints/test_info.py (1)
25-25: LGTM! Test configuration correctly updated.The change from
"feedback_disabled": Trueto"feedback_enabled": Falsecorrectly maintains the same test scenario (feedback disabled) while using the improved positive naming convention.tests/unit/__init__.py (1)
21-21: LGTM! Configuration correctly updated.The change from
"transcripts_disabled": Trueto"transcripts_enabled": Falsecorrectly maintains the same functional behavior (transcripts disabled) while using the improved positive naming convention.src/app/endpoints/feedback.py (1)
47-47: Excellent refactoring to eliminate double negative.The change from
not feedback_disabledtofeedback_enabledsignificantly improves code readability and eliminates the confusing double negative pattern. The function name already implied positive logic, and now the implementation is consistent with that intent.tests/unit/app/endpoints/test_config.py (1)
43-43: Correct test update for configuration refactoring.The change from
"feedback_disabled": Trueto"feedback_enabled": Falseproperly maintains the same test logic (feedback is disabled) while using the new positive naming convention. The boolean value inversion is correct.tests/unit/utils/test_common.py (1)
44-44: Consistent test updates across all MCP server test functions.All instances of
UserDataCollection(feedback_disabled=True)have been correctly updated toUserDataCollection(feedback_enabled=False), maintaining the same test logic (feedback is disabled) while using the new positive naming convention. The changes are consistent across all test functions.Also applies to: 84-84, 128-128, 175-175, 229-229, 274-274
src/app/endpoints/query.py (1)
72-72: Consistent application of double negative elimination for transcripts.The change from
not transcripts_disabledtotranscripts_enabledmirrors the same improvement made to the feedback functionality. This eliminates the confusing double negative pattern and makes the implementation consistent with the positive function nameis_transcripts_enabled().docs/openapi.md (1)
783-783: Documentation correctly updated to reflect configuration changes.The field name changes from
feedback_disabledtofeedback_enabledandtranscripts_disabledtotranscripts_enabledproperly update the API documentation to match the configuration model refactoring. This ensures the documentation remains accurate and consistent with the implementation.Also applies to: 785-785
src/app/endpoints/config.py (1)
39-41: LGTM: Configuration flag refactoring correctly implementedThe changes successfully eliminate double negatives by:
- Converting
feedback_disabled: Falsetofeedback_enabled: True(semantically equivalent)- Converting
transcripts_disabled: Truetotranscripts_enabled: False(semantically equivalent)The boolean value inversions maintain equivalent functionality while improving configuration clarity.
tests/unit/app/endpoints/test_models.py (1)
54-54: LGTM: Test configurations updated to use positive flagsThe test configurations have been correctly updated to use the new positive flag naming:
transcripts_disabled→transcripts_enabledfeedback_disabled→feedback_enabledThese changes align with the codebase-wide refactoring and maintain test semantics with appropriate boolean value inversions.
Also applies to: 98-98, 137-137
docs/config.puml (1)
66-68: LGTM: Documentation updated to reflect positive flag namingThe PlantUML class diagram has been correctly updated to reflect the new positive field names:
feedback_disabled→feedback_enabledtranscripts_disabled→transcripts_enabledThis keeps the documentation in sync with the codebase refactoring.
tests/unit/app/endpoints/test_streaming_query.py (1)
97-97: LGTM: Test fixture updated for positive flag namingThe configuration in
setup_configuration_fixturehas been correctly updated:
"transcripts_disabled": True→"transcripts_enabled": FalseThe boolean inversion maintains the same semantic meaning (transcripts effectively disabled) while using the new positive flag naming convention.
tests/unit/app/endpoints/test_conversations.py (1)
41-41: LGTM: Test fixture configuration updated consistentlyThe
setup_configuration_fixturehas been updated to use the positive flag naming:
"transcripts_disabled"→"transcripts_enabled": FalseThis maintains semantic equivalence while following the consistent refactoring pattern applied across all test files.
README.md (3)
109-112: LGTM! Clear improvement in configuration naming.The refactor from
feedback_disabled: falseandtranscripts_disabled: falsetofeedback_enabled: trueandtranscripts_enabled: trueeliminates confusing double negatives while maintaining equivalent functionality. This makes the configuration more intuitive and consistent with theenabled_*naming convention mentioned in the PR objectives.
188-192: Consistent configuration update across examples.The Llama Stack client library configuration example correctly applies the same positive flag naming convention, maintaining consistency throughout the documentation.
440-444: Data collector service configuration properly updated.The data collector service configuration section correctly reflects the new positive flag naming, ensuring users have consistent documentation across all configuration examples.
tests/unit/app/endpoints/test_feedback.py (3)
18-19: Test correctly updated for positive flag semantics.The test properly uses
feedback_enabled = Trueinstead of the previousfeedback_disabled = False, maintaining the same logical behavior while using the clearer positive flag naming.
24-25: Disabled state test correctly updated.The test for the disabled state properly sets
feedback_enabled = False, which is logically equivalent to the previousfeedback_disabled = Truebut much clearer to understand.
130-134: Feedback status test aligned with new flag.The status test correctly uses
feedback_enabled = Trueand validates that the response shows{"enabled": True}, ensuring proper integration with the refactored configuration model.tests/unit/test_configuration.py (4)
68-69: Configuration test correctly updated for positive flag semantics.The test properly replaces
"feedback_disabled": Truewith"feedback_enabled": False, maintaining equivalent functionality while using clearer positive flag naming. The corresponding assertion correctly checks forfeedback_enabled is False.Also applies to: 99-99
120-121: MCP server configuration test properly updated.The test configuration dictionary correctly uses the new positive flag naming while maintaining the same logical state (feedback disabled).
166-167: YAML configuration tests consistently updated.The YAML configuration strings correctly use
feedback_enabled: falseinstead offeedback_disabled: true, maintaining consistency between dictionary and YAML-based configuration tests.Also applies to: 198-199
238-239: All remaining configuration tests properly aligned.All test scenarios consistently use the new
feedback_enabled: falseconfiguration, ensuring comprehensive test coverage of the refactored configuration model.Also applies to: 269-270, 376-377, 416-417
src/services/data_collector.py (3)
98-99: Feedback collection logic correctly updated.The condition properly changes from checking if feedback is disabled to checking if feedback is not enabled (
not feedback_enabled), maintaining equivalent functionality while using clearer positive flag semantics.
111-112: Transcript collection logic consistently updated.The transcript collection follows the same pattern as feedback collection, correctly using
not transcripts_enabledinstead of the previoustranscripts_disabledcheck.
226-227: Directory cleanup logic properly aligned.The empty directory cleanup logic correctly checks for
not transcripts_enabled, ensuring consistency with the transcript collection logic and maintaining the same functional behavior.tests/unit/services/test_data_collector.py (3)
54-55: Feedback collection tests correctly updated.The mock configuration properly uses the new
feedback_enabledflag with appropriate boolean values to test both enabled and disabled scenarios.Also applies to: 64-65, 89-90
106-107: Transcript collection tests properly updated.The transcript-related tests correctly use
transcripts_enabledwith appropriate boolean values to test various scenarios including disabled state, enabled with missing directory, and successful collection.Also applies to: 116-117, 132-133
491-492: Directory cleanup tests consistently updated.All directory cleanup tests properly use the new
transcripts_enabledflag with correct boolean values to test both enabled and disabled scenarios for the cleanup functionality.Also applies to: 500-501, 530-531, 562-563
tests/unit/app/endpoints/test_query.py (3)
51-51: LGTM! Configuration fixture correctly updated.The change from
"transcripts_disabled": Trueto"transcripts_enabled": Falsemaintains equivalent functionality while eliminating the double negative pattern.
89-94: LGTM! Test correctly updated for positive flag.The test now patches
transcripts_enabled=Trueinstead of the old disabled flag, which correctly tests the enabled scenario.
119-121: LGTM! Mock configuration correctly updated.The mock configuration now uses
transcripts_enabledwith the appropriate boolean value, maintaining the same test behavior while using the new positive flag naming.src/models/config.py (2)
121-124: LGTM! Field definitions correctly refactored.The changes successfully eliminate double negatives by renaming
feedback_disabled/transcripts_disabledtofeedback_enabled/transcripts_enabledwith appropriately inverted default values, maintaining equivalent functionality.
130-136: LGTM! Validation logic correctly updated.The validation conditions now check the positive flags (
feedback_enabledandtranscripts_enabled) instead of negating the old disabled flags, maintaining equivalent logic while improving readability.tests/unit/models/test_config.py (6)
122-125: LGTM! Test constructor and assertions correctly updated.The test now uses
feedback_enabled=Falseinstead of the old disabled flag, with the assertion correctly checking the new field name.
135-135: LGTM! Exception test correctly updated.The test now uses
feedback_enabled=Trueto trigger the validation error, which is correct since the validation now checks for enabled flags requiring storage paths.
141-141: LGTM! Transcripts test correctly updated.The test constructor now uses
transcripts_enabled=Falseinstead of the old disabled flag.
152-152: LGTM! Transcripts exception test correctly updated.The test now uses
transcripts_enabled=Trueto trigger the validation error, which is correct for the new validation logic.
320-321: LGTM! Configuration constructor calls consistently updated.All UserDataCollection constructor calls now use
feedback_enabled=Falseinstead of the old disabled flag, maintaining consistency across all Configuration test scenarios.Also applies to: 342-343, 370-371, 393-394, 476-477, 562-563
438-440: LGTM! JSON dump assertions correctly updated.The expected JSON output now reflects the new field names (
feedback_enabled,transcripts_enabled) with appropriate boolean values, ensuring the dump tests pass with the refactored model.Also applies to: 516-518, 608-610
docs/openapi.json (1)
254-257: Example payload now contradicts schema defaults
feedback_enabledisfalsewhile the schema default istrue, andtranscripts_enabledistruewhile (after fix) the default should befalse. Make the example reflect the intended defaults to avoid confusion.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Thanks, when I was working on the feedback bits I mostly copied from the options we had in road-core/service but, I do agree that it is not the best and I prefer naming the options as this patch suggests. So it's a +1 from me
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
How it would be compatible with OLS? It seems to break 1-on-1 feature pairing, so OLS would need to be prepared for this change. Do you have an issue created for it?
|
@tisnik https://issues.redhat.com/browse/OLS-1952 good from OLS side now. |
…tspeed-core#271) * Change double negative in user data collection config * Update tests/unit/services/test_data_collector.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update tests/unit/app/endpoints/test_query.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tspeed-core#271) * Change double negative in user data collection config * Update tests/unit/services/test_data_collector.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update tests/unit/app/endpoints/test_query.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
Description
This poped up a few times in the past, why is it defined as a double negative. The rest of the configuration uses
enabled_*so lets change it to be consistent.Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Documentation
feedback_enabledandtranscripts_enabledflags instead offeedback_disabledandtranscripts_disabled.Refactor
feedback_enabled,transcripts_enabled) for user data collection features.Tests