Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feedback: allow events coming from roleGen #1502

Merged

Conversation

goneri
Copy link
Contributor

@goneri goneri commented Jan 22, 2025

Accept the action and feeback events coming from the extension for the role generation.

@goneri goneri marked this pull request as draft January 22, 2025 15:22
@goneri goneri force-pushed the goneri/feedback-allow-events-coming-from-roleGen_31315 branch from 0e1e490 to 2c9345f Compare January 22, 2025 15:26
@goneri goneri force-pushed the goneri/feedback-allow-events-coming-from-roleGen_31315 branch 2 times, most recently from 5e3191f to 4396f22 Compare January 22, 2025 16:17
@goneri goneri force-pushed the goneri/feedback-allow-events-coming-from-roleGen_31315 branch 6 times, most recently from 55c1d14 to 919fafe Compare January 23, 2025 18:28
@goneri goneri marked this pull request as ready for review January 23, 2025 19:12
@goneri goneri requested review from mabashian and manstis January 23, 2025 19:12
Copy link
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a couple of niggles.. but I think I answered one myself; LGTM 👍

I'd more interested in your if False... when sending page transition events.

ansible_ai_connect/ai/api/serializers.py Outdated Show resolved Hide resolved
ansible_ai_connect/ai/api/views.py Show resolved Hide resolved
@goneri goneri force-pushed the goneri/feedback-allow-events-coming-from-roleGen_31315 branch 3 times, most recently from 753ec5d to 29cb634 Compare January 28, 2025 17:16
@goneri goneri requested a review from manstis January 28, 2025 19:39
mabashian
mabashian previously approved these changes Jan 29, 2025
Copy link
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the whole LGTM 👍

I just don't understand however why the test data is duplicated.

Am I missing something obvious?!?

def test_feedback_generation(self):
action = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to understand why the properties in this structure are duplicated.

If it's important that it's duplicated perhaps add a comment so somebody doesn't think to "clean it up" in the future.

Please explain yourself 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, it's me. Let's fix that.

Accept the action and feeback events coming from the extension for the role generation.
@goneri goneri force-pushed the goneri/feedback-allow-events-coming-from-roleGen_31315 branch from 29cb634 to 0dcac2a Compare February 4, 2025 13:05
@goneri goneri requested review from manstis and mabashian February 4, 2025 13:06
Copy link
Contributor

@manstis manstis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

LGTM 👍

🤣

Copy link

sonarqubecloud bot commented Feb 4, 2025

@goneri goneri merged commit b2e04ee into main Feb 4, 2025
11 checks passed
@goneri goneri deleted the goneri/feedback-allow-events-coming-from-roleGen_31315 branch February 4, 2025 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants