Skip seated user check for chat segment events #1500
Conversation
| @@ -181,6 +181,10 @@ def redact_seated_users_data(event: Dict[str, Any], allow_list: Dict[str, Any]) | |||
| Returns: | |||
| - dict: A new dictionary containing only the allowed nested keys from the source dictionary. | |||
| """ | |||
| # If an asterisk is found in the allow list, just return the given event dict to allow all keys. | |||
| if "*" in allow_list: | |||
There was a problem hiding this comment.
It'd be nice to have a test for these with your new check?
I know SonarCloud says you have 100%... but is that a false positive? I don't see any.
There was a problem hiding this comment.
I have added two lines to test_operational_telemetry()`
self.user.rh_user_has_seat = True
self.user.organization = Organization.objects.get_or_create(id=1)[0]
which forces to run the test case with a "seated" user. Without this allow_list change, the test case failed. Isn't this change enough for the test coverage?
There was a problem hiding this comment.
Isn't this change enough for the test coverage?
I found there were some test cases on redact operations in test_segment.py. I'll add two test cases to it.
There was a problem hiding this comment.
Yes, that's fine @TamiTakamiya
It wasn't clear from your PR.
There was a problem hiding this comment.
Hey @TamiTakamiya, @manstis, sorry for late response, just my 2c the idea behind allow list agains block list in this case is making sure you are logging only something you really need to log, and avoid situation we are storing things we are not supposed to store.
Whit the asterisk it is easier to make a mistake and store unintended data.
There was a problem hiding this comment.
@hasys You're never late to the party and, as author of the original white listing, your opinion is important.. When you wrote the enhancement you no doubt had a clear vision in mind and it should not be bastardized to fit other use-cases (as this is how we end up with spaghetti code!)
What would you have proposed?
IIUC the issue was that the Chat Bot events were not recognised as "valid" events and hence filtered out.
@TamiTakamiya's change means they are no longer blocked by the filter and obviates the need to list all fields.
There was a problem hiding this comment.
Approving although agree with @manstis , tests for new wildcard would be great, that's an important feature... Anyway I have tested same fix locally and that's the right fix. Thanks @TamiTakamiya !
| @@ -181,6 +181,10 @@ def redact_seated_users_data(event: Dict[str, Any], allow_list: Dict[str, Any]) | |||
| Returns: | |||
| - dict: A new dictionary containing only the allowed nested keys from the source dictionary. | |||
| """ | |||
| # If an asterisk is found in the allow list, just return the given event dict to allow all keys. | |||
| if "*" in allow_list: | |||
There was a problem hiding this comment.
Yes, that's fine @TamiTakamiya
It wasn't clear from your PR.
|
TamiTakamiya
deleted the
TamiTakamiya/skip-seat-user-check-for-chat-segment-events
branch
Jira Issue: n/a
Description
Skip "seated user check" for Chat-related segment events because Chat is irrelevant to "seated users" or WCA.
I added two new entries to
allow_list:The line
"*": None,implies all properties are allows.The problem was found on the
chatOperationalEvent, but I have added a fix for thechatFeedbackEventas well.Testing
Steps to test
Scenarios tested
Unit tests + manual tests by running local AI Connect server.
Production deployment