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

Revert "Revert "add simple audit log to levels"" #15019

Merged
merged 3 commits into from May 15, 2017

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented May 11, 2017

Reverts #14999

The original PR changed the levelbuilder controller to use assign_attributes plus save rather than update; unfortunately, DSL-Defined levels were overriding update to add some custom functionality; converting them to instead override assign_attributes is the simple fix.

@Hamms Hamms force-pushed the revert-14999-revert-14719-add-level-audit-log branch from e294d5d to c1b5540 Compare May 15, 2017 19:42
@Hamms Hamms requested a review from islemaster May 15, 2017 21:18
@Bjvanminnen
Copy link
Contributor

Are there times when we would validly be calling update (and not getting common functionality) on DSL defined levels? If we should always be calling our custom functionality (currently in assign_attributes) should we override the update method to throw, so that other people don't call the wrong method in the future and not realize they're missing this custom logic?

@Hamms
Copy link
Contributor Author

Hamms commented May 15, 2017

@Bjvanminnen
Copy link
Contributor

👍

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

👍 Lookin' good.

@Hamms Hamms merged commit ef1dacb into staging May 15, 2017
@Hamms Hamms deleted the revert-14999-revert-14719-add-level-audit-log branch May 15, 2017 22:06
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.

None yet

3 participants