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.

Sign up for GitHub

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

Jump to bottom

fix: change request audit logs #2527

Merged
merged 6 commits into from
Jul 27, 2023
Merged

fix: change request audit logs #2527

merged 6 commits into from
Jul 27, 2023

Conversation

gagantrivedi
Copy link
Member

@gagantrivedi gagantrivedi commented Jul 27, 2023 •
edited

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

  • I have run pre-commit to check linting
  • 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

  • Add audit log for change request deletion
  • Avoid creating feature state audit logs for uncommitted change request

How did you test this code?

Tests: https://github.com/Flagsmith/flagsmith-workflows/pull/21/files

@vercel
Copy link

vercel bot commented Jul 27, 2023 •
edited

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 27, 2023 10:00am
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 27, 2023 10:00am
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 27, 2023 10:00am

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

github-actions bot commented Jul 27, 2023 •
edited

Uffizzi Preview deployment-31907 was deleted.

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023 •
edited

Codecov Report

Patch coverage: 86.36% and project coverage change: -0.01% ⚠️

Comparison is base (a111976) 95.43% compared to head (785026a) 95.42%.
Report is 6 commits behind head on main.

❗ Current head 785026a differs from pull request most recent head 51285d0. Consider uploading reports for the commit 51285d0 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2527      +/-   ##
==========================================
- Coverage   95.43%   95.42%   -0.01%     
==========================================
  Files         965      966       +1     
  Lines       27136    27149      +13     
==========================================
+ Hits        25897    25908      +11     
- Misses       1239     1241       +2
Files Changed Coverage Δ
api/features/multivariate/models.py 93.15% <50.00%> (-1.22%) ⬇️
api/features/workflows/core/models.py 95.27% <50.00%> (-0.76%) ⬇️
api/audit/tasks.py 94.33% <80.00%> (-1.74%) ⬇️
api/audit/constants.py 100.00% <100.00%> (ø)
api/features/models.py 93.58% <100.00%> (+0.32%) ⬆️
...i/tests/unit/features/test_unit_features_models.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@dabeeeenster dabeeeenster changed the title Fix/cr audit logs fix: cr audit logs Jul 27, 2023
@gagantrivedi gagantrivedi changed the title fix: cr audit logs fix: change request audit logs Jul 27, 2023
@gagantrivedi gagantrivedi marked this pull request as ready for review July 27, 2023 07:43
matthewelwell
matthewelwell approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

Approved with one minor comment.

api/features/models.py
Comment on lines +883 to +890
def get_audit_log_related_object_id(self, history_instance) -> int:
# Change requests can create, update, or delete feature states that may never go live,
# since we already include the change requests in the audit log
# we don't want to create separate audit logs for the associated
# feature states
if self.belongs_to_uncommited_change_request:
return None
return super().get_audit_log_related_object_id(history_instance)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're now shifting this logic, I wonder if we should reorder the logic in the create_audit_log_from_historical_record task function so that we get the related object id first? Just because (I think) in general the log messages require more database queries to generate.

i.e. here: https://github.com/Flagsmith/flagsmith/blob/main/api/audit/tasks.py#L87-L94

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about doing it

@dabeeeenster dabeeeenster merged commit d7c459e into main Jul 27, 2023
17 checks passed
@dabeeeenster dabeeeenster deleted the fix/cr-audit-logs branch July 27, 2023 14:09
@dabeeeenster dabeeeenster mentioned this pull request Jul 27, 2023
Merged
@dabeeeenster dabeeeenster mentioned this pull request Aug 22, 2023
Closed
@dabeeeenster dabeeeenster mentioned this pull request Aug 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewelwell matthewelwell matthewelwell approved these changes

@novakzaballa novakzaballa Awaiting requested review from novakzaballa novakzaballa was automatically assigned from Flagsmith/flagsmith

Assignees
No one assigned
Labels
api Issue related to the REST API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gagantrivedi @codecov-commenter @matthewelwell @dabeeeenster