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

[8164] apps/ai_reports: connect with external API #54

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

goapunk
Copy link
Contributor

@goapunk goapunk commented May 30, 2024

  • add a celery task which sends a comment to the XAI server and saves the response as AiReport
  • connect comment post_save signal with celery task
  • rename category to label to be in line with the XAI response
  • change AiReport fields to JSONField for now
  • add tests

@goapunk goapunk requested a review from m4ra May 30, 2024 13:34
@goapunk goapunk force-pushed the jd-2024-05-connect-xai branch 2 times, most recently from d97bf33 to d3d8ea0 Compare May 30, 2024 14:45
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

thank you, some small improvements and will be good to go!

apps/ai_reports/signals.py Outdated Show resolved Hide resolved
apps/ai_reports/signals.py Outdated Show resolved Hide resolved
if getattr(settings, "XAI_API_URL", None):
comment_text_changed = getattr(instance, "_former_comment") != getattr(
instance, "comment"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't use here the update_fields to check what's has been updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as far as I understand update_fields can be:

  1. None - > everything will be saved
  2. A list of fields -> only the fields in the list will be saved
  3. Empty list -> nothing will be saved

So we can't get rid of the check in L19 as we don't know if the text changed in case 1

apps/ai_reports/signals.py Outdated Show resolved Hide resolved
apps/ai_reports/signals.py Outdated Show resolved Hide resolved
apps/ai_reports/tasks.py Show resolved Hide resolved
apps/ai_reports/tasks.py Outdated Show resolved Hide resolved
changelog/8164.md Show resolved Hide resolved
tests/ai_reports/factories.py Outdated Show resolved Hide resolved
@goapunk goapunk requested a review from m4ra June 3, 2024 11:56
@goapunk goapunk force-pushed the jd-2024-05-connect-xai branch 2 times, most recently from e533682 to 103582a Compare June 3, 2024 12:07
@goapunk goapunk changed the title [8164] apps/ai_reports: connect with external API WIP: [8164] apps/ai_reports: connect with external API Jun 3, 2024
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

An extra test is needed to make sure the perform_update view calls the post_save() signal

)
if created or comment_text_changed:
# FIXME: use delay_on_commit() once updated to celery 5.x
get_classification_for_comment.delay(instance.pk)
Copy link
Contributor

Choose a reason for hiding this comment

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

the update needs a test for when comment is updated via the views, which comes from django rest api, and not sure if it performs an object save or update query. We only have a GET request test in tests/moderatorfeedback/test_moderator_comment_feedback_api.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added some tests

- add a celery task which sends a comment to the XAI server and saves
  the response as AiReport
- connect comment post_save signal with celery task
- rename category to label to be in line with the XAI response
- change AiReport fields to JSONField for now
- add tests
- **BREAKING CHANGE** Reset migrations for the ai_reports app (see
  changelog)
@goapunk goapunk requested a review from m4ra June 3, 2024 14:54
@goapunk goapunk changed the title WIP: [8164] apps/ai_reports: connect with external API [8164] apps/ai_reports: connect with external API Jun 3, 2024
@goapunk
Copy link
Contributor Author

goapunk commented Jun 3, 2024

@m4ra I also added manual migrations so we don't need to wipe the database for the ai_reports app, as there already was some manually created content on dev

Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

very nice, thank you!

@m4ra m4ra merged commit 484a851 into main Jun 3, 2024
2 of 3 checks passed
@m4ra m4ra deleted the jd-2024-05-connect-xai branch June 3, 2024 15:21
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.

2 participants