-
Notifications
You must be signed in to change notification settings - Fork 84
Conversation
@ben181231, thanks for your PR! By analyzing the annotation information on this pull request, we identified @rickmak to be a potential reviewer |
0e28c90
to
4676178
Compare
4676178
to
5138c85
Compare
@cheungpat Please help to review this PR. |
|
||
import "github.com/skygeario/skygear-server/pkg/server/logging" | ||
|
||
var log = logging.LoggerEntry("plugin-evnet") |
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.
s/plugin-evnet/plugin-event/
@@ -123,6 +117,12 @@ func TestRun(t *testing.T) { | |||
So(string(out), ShouldEqual, `"op hello:world"`) | |||
}) | |||
|
|||
Convey("event", func() { | |||
out, err := transport.SendEvent("foo:bar", []byte{}) |
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.
prefer foo-bar here
if err != nil { | ||
panic(fmt.Sprintf("Unable to get registration info from plugin. Error: %v", err)) | ||
// SendEvents sends event to all plugins | ||
func (c *Context) SendEvents(name string, data []byte, async bool) { |
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 would prefer SendEvent
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.
need to mention explicitly in the comment that async
being false
means the event messages are sent to each plugin one after or another, or that event messages are sent to all plugins at once, then wait for all operations to complete
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.
Okay. I will update the comment.
Just adding a note that we still need schema-change event in RecordSaveHandler. |
3b926b7
to
c24c83f
Compare
@cheungpat The PR is updated. |
@@ -345,6 +347,17 @@ func (h *RecordSaveHandler) Handle(payload *router.Payload, response *router.Res | |||
results = append(results, result) | |||
} | |||
|
|||
if resp.SchemaUpdated && h.EventSender != nil { |
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.
move this section out of this function scope (incl or excl the if statement
@@ -51,8 +51,8 @@ type schemaField struct { | |||
TypeName string `mapstructure:"type" json:"type"` | |||
} | |||
|
|||
func (resp *schemaResponse) Encode(data map[string]skydb.RecordSchema) { | |||
resp.Schemas = make(map[string]schemaFieldList) | |||
func encodeRecordSchemas(data map[string]skydb.RecordSchema) map[string]schemaFieldList { |
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.
Move this to recordutil or (new) schemautil.
Basically we try not to have different handler files (record and schema) with code calling code in other file.
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 see.
return schemaMap | ||
} | ||
|
||
func sendSchemaChangedEvent( |
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.
good idea to move this in a recordutil/schemautil file
@@ -28,7 +28,7 @@ func TestExtend(t *testing.T) { | |||
db := c.PublicDB().(*database) | |||
|
|||
Convey("return Resemble RecordSchema on second call", func() { | |||
err := db.Extend("note", skydb.RecordSchema{ | |||
_, err := db.Extend("note", skydb.RecordSchema{ |
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.
This test file tests Extend
. At the very least you want to test the boolean return value from the Extend
function.
6903d38
to
63e5cb1
Compare
good job benlei! |
connect #199