-
-
Notifications
You must be signed in to change notification settings - Fork 70
audit tag renames #1648
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
audit tag renames #1648
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I should probably figure out how to add a test, but there's no merge test for me to piggyback on so I'm a bit lost. I didn't find any existing tests that check the audit log. I do not understand the system-test failure (is it spurious?). |
Yeah, system tests are just flaky, it's annoying, but for now until I or someone else sits down and fixes them, the only remedy is to rerun the workflow whenever it fails on CircleCI (https://app.circleci.com/pipelines/github/codidact - you should have access to it via the org - just "rerun from failed" if no one's around to do so - might take a couple of tries, though): |
Adding tests for audit logs should be relatively straightforward: just test the controller method in question and then ask the model to get you the most recent audit log of the relevant type - even checking that it's there at all should be enough |
…ised on falsy status
I've added the test (not for the audit log, but for the controller method - it's more important to have the base functionality covered - if you want to get some practice, feel free to add one specifically for the log [best to just expand the test I've made] - do ping me if you need help with that) and several fixes (will comment on them separately). |
status = false | ||
|
||
Post.transaction do | ||
@tag.transaction do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got Post.transaction
from merge
a few lines down. Do we need to change it there too? Or is that one correct because tag merges modify posts?
Those fixes were educational. Thank you! |
…rpowers that would mask a problem for mods
5a9f058
to
88b58ca
Compare
Adds tag renames to the audit log -- merges are already there, and renames, like merges, don't leave visible history other than the log. Fixes #1638.