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

"global" run time enable (2) #61

Merged
merged 9 commits into from
Apr 16, 2019

Conversation

webmasterMeyers
Copy link
Contributor

Continuation from #59, Code is updated as you requested. I have not yet added any unit-tests. Not sure how, yet. But this works as manually tested.

This is also much simpler!

Copy link
Owner

@DamienHarper DamienHarper left a comment

Choose a reason for hiding this comment

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

@webmasterMeyers A few points:

  • Please, replace the AuditConfiguration::setEnabled(bool $enabled) with two separate methods AuditConfiguration::enable() and AuditConfiguration::disable() or AuditConfiguration::enableAudit() and AuditConfiguration::disableAudit() as you like.
  • Make these methods fluent (return current instance to allow method chaining)
  • Checkout this dedicated README for more information about tests -- you may start by editing tests/DoctrineAuditBundle/AuditConfigurationTest.php and add a new test methods AuditConfigurationTest.php ::testIsAuditedWhenAuditIsDisabled() (to test isAudited method when global enabled flag is false) and AuditConfigurationTest.php ::testIsAuditedWhenAuditIsEnabled() (to test isAudited method when global enabled flag is true).

@webmasterMeyers
Copy link
Contributor Author

  • methods are fluent!
  • 2 methods, enable & disable (sorry, meant to do it that way... old habits die hard)
  • please double check the tests are correct/enough

@DamienHarper
Copy link
Owner

Please, do run code coverage once your tests are done.

Then, open tests/coverage/index.html with your favorite browser and click on the tested file(s) you made tests for (AuditConfiguration.php in your case) and look at what is highlighted in pink: it's missing code coverage.

@DamienHarper DamienHarper merged commit 326f67b into DamienHarper:master Apr 16, 2019
@webmasterMeyers
Copy link
Contributor Author

Thanks for walking me through all that!

@webmasterMeyers webmasterMeyers deleted the globalEnable branch April 16, 2019 14:42
@DamienHarper
Copy link
Owner

My pleasure

@webmasterMeyers webmasterMeyers restored the globalEnable branch April 16, 2019 14:45
@DamienHarper DamienHarper added the enhancement New feature or request label Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants