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

fix(webhook/signal): Optimise historical record writes #5041

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gagantrivedi
Copy link
Member

@gagantrivedi gagantrivedi commented Jan 27, 2025

Thanks for submitting a PR! Please check the boxes below:

  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

  • Subclass HistoricalRecords to not write ~ historical records on soft delete
  • Add get_skip_create_audit_log to the signal method to avoid creating tasks that don't do anyting
  • Add history to SegmentRule
  • Update django-lifecycle to support more complex hook conditions

How did you test this code?

Adds and updates unit tests

Copy link

vercel bot commented Jan 27, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview Jan 27, 2025 9:03am
flagsmith-frontend-preview ⬜️ Ignored (Inspect) Visit Preview Jan 27, 2025 9:03am
flagsmith-frontend-staging ⬜️ Ignored (Inspect) Visit Preview Jan 27, 2025 9:03am

@github-actions github-actions bot added the api Issue related to the REST API label Jan 27, 2025
Copy link
Contributor

github-actions bot commented Jan 27, 2025

Uffizzi Ephemeral Environment deployment-60297

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/5041

📄 View Application Logs etc.

What is Uffizzi? Learn more!

@gagantrivedi gagantrivedi changed the title Fix/audit logs fix(wip): fix audit logs Jan 27, 2025
@github-actions github-actions bot added the fix label Jan 27, 2025
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.39%. Comparing base (2faca89) to head (1fbfef9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5041      +/-   ##
==========================================
- Coverage   97.40%   97.39%   -0.01%     
==========================================
  Files        1193     1193              
  Lines       41675    41717      +42     
==========================================
+ Hits        40593    40632      +39     
- Misses       1082     1085       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gagantrivedi gagantrivedi changed the title fix(wip): fix audit logs fix(webhook/signal): Optimise historical record writes Jan 27, 2025
@gagantrivedi gagantrivedi requested review from a team and khvn26 and removed request for a team January 27, 2025 08:53
@@ -134,6 +135,18 @@ def schedule_hubspot_tracking(self) -> None:
),
)

@hook(AFTER_SAVE, condition=(WhenFieldHasChanged("email", has_changed=True)))
def send_warning_email(self):
from users.tasks import send_email_changed_notification_email
Copy link
Member Author

Choose a reason for hiding this comment

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

We can certainly get rid of this local import by doing some refactoring, but I will create a different pull request for that to avoid polluting this one

Copy link
Member

Choose a reason for hiding this comment

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

I'm so used to importing tasks this way I regard it basically as a pattern now.

Globally, we could think on implementing a task dispatcher that accepts a string and does this under the hood so the task users don't have to.

@gagantrivedi gagantrivedi linked an issue Jan 27, 2025 that may be closed by this pull request
4 tasks
@gagantrivedi gagantrivedi marked this pull request as ready for review January 27, 2025 08:55
@gagantrivedi gagantrivedi requested a review from a team as a code owner January 27, 2025 08:55
@github-actions github-actions bot added fix and removed fix labels Jan 27, 2025
Copy link
Contributor

github-actions bot commented Jan 27, 2025

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-e2e:pr-5041 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api-test:pr-5041 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api:pr-5041 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-frontend:pr-5041 Finished ✅ Results
ghcr.io/flagsmith/flagsmith:pr-5041 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-private-cloud:pr-5041 Finished ✅ Results

@github-actions github-actions bot added fix and removed fix labels Jan 27, 2025
@matthewelwell matthewelwell removed the request for review from a team January 27, 2025 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleted feature throws FLAG_UPDATED event
2 participants