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

add commit callback #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

pv97
Copy link

@pv97 pv97 commented Jun 18, 2018

Tests were not running locally, so updated wercker.yaml

Add After<Create/Update/Save/Delete>Commit callbacks and updated tests for callbacks.

Right now tests are failing because running tests doesn't build the latest changes for some reason. If I comment out all the old callbacks, they are still being called.

Dont merge yet btw.

@pv97
Copy link
Author

pv97 commented Jun 18, 2018

WOW, even though the test files and source files are in the same directory, they have a different package gorm and gorm_tests. And gorm_tests import github.com/jinzhu/gorm, so it doesn't even use the local source files for testing -_-

@@ -1,2 +1,3 @@
documents
_book
.wercker

Choose a reason for hiding this comment

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

newline?

if !scope.HasError() {
scope.CallMethod("AfterUpdateCommit")
}
if !scope.HasError() {

Choose a reason for hiding this comment

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

Why do two different checks for the same thing?

Copy link
Author

Choose a reason for hiding this comment

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

i am just following convention :D but maybe scope.CallMethod could result in changing HasError

Choose a reason for hiding this comment

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

oooooooooooooooo. good fucking point.

// afterUpdateCommitCallback will invoke `AfterUpdateCommit`, `AfterSaveCommit` method after update commit
func afterUpdateCommitCallback(scope *Scope) {
if _, ok := scope.Get("gorm:update_column"); !ok {
if !scope.HasError() {

Choose a reason for hiding this comment

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

If it has an error, is this somehow logged/or thrown to the api user at any point?

Copy link
Author

Choose a reason for hiding this comment

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

it should return error to where you called the DB transaction like Save or Update

@@ -53,6 +61,11 @@ func (s *Product) AfterSave() (err error) {
return
}

func (s *Product) AfterSaveCommit() {

Choose a reason for hiding this comment

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

Is the difference between update and save actually update and create? Basically does save only mean create?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why they have it this way, but calling Create both creates and saves, and calling Update both updates and saves

@@ -18,6 +18,7 @@ func init() {
DefaultCallback.Update().Register("gorm:save_after_associations", saveAfterAssociationsCallback)
DefaultCallback.Update().Register("gorm:after_update", afterUpdateCallback)
DefaultCallback.Update().Register("gorm:commit_or_rollback_transaction", commitOrRollbackTransactionCallback)
DefaultCallback.Update().Register("gorm:after_update_commit", afterUpdateCommitCallback)

Choose a reason for hiding this comment

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

Interesting. So this is already an event?

Copy link
Author

Choose a reason for hiding this comment

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

They will call whatever event you add in order.

Choose a reason for hiding this comment

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

So just by registering you're creating an event as well?

Copy link
Author

Choose a reason for hiding this comment

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

yea

Choose a reason for hiding this comment

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

but then how is it to know that this should be triggered AFTER commit? And shouldn't there be a gorm:after_create_commit?

Copy link
Author

Choose a reason for hiding this comment

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

they are called in the order they are defined here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants