{"action":"created","issue":{"url":"https://api.github.com/repos/deployd/deployd/issues/769","repository_url":"https://api.github.com/repos/deployd/deployd","labels_url":"https://api.github.com/repos/deployd/deployd/issues/769/labels{/name}","comments_url":"https://api.github.com/repos/deployd/deployd/issues/769/comments","events_url":"https://api.github.com/repos/deployd/deployd/issues/769/events","html_url":"https://github.com/deployd/deployd/issues/769","id":183478029,"number":769,"title":"Make put() and post() more similar","user":{"login":"Michael-Hamburger","id":7239350,"avatar_url":"https://avatars.githubusercontent.com/u/7239350?v=3","gravatar_id":"","url":"https://api.github.com/users/Michael-Hamburger","html_url":"https://github.com/Michael-Hamburger","followers_url":"https://api.github.com/users/Michael-Hamburger/followers","following_url":"https://api.github.com/users/Michael-Hamburger/following{/other_user}","gists_url":"https://api.github.com/users/Michael-Hamburger/gists{/gist_id}","starred_url":"https://api.github.com/users/Michael-Hamburger/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Michael-Hamburger/subscriptions","organizations_url":"https://api.github.com/users/Michael-Hamburger/orgs","repos_url":"https://api.github.com/users/Michael-Hamburger/repos","events_url":"https://api.github.com/users/Michael-Hamburger/events{/privacy}","received_events_url":"https://api.github.com/users/Michael-Hamburger/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2016-10-17T17:30:05Z","updated_at":"2016-10-26T11:34:00Z","closed_at":"2016-10-26T10:49:23Z","body":"At first I thought about asking this on gitter but after writing everything out it is longer than I thought. So here it is:\r\n\r\n```Put``` and ```Post``` are similar. ```Post``` creates a new entry in the collection, ```Put``` updates an already existing entry in the collection. In fact ```Put``` only needs to load the already existing entry and just merges the changes. A coworker mentioned to me that the way deployd handles ```Put``` and ```Post``` is different and I looked at how they are are implemented. They are both methods in https://github.com/deployd/deployd/blob/master/lib/resources/collection/index.js.\r\n\r\n```Put``` queries the entry, merges the changes, validates the collection (on validate event) and after the validation doesn't throw any error runPutEvent() is called.\r\nrunPutEvent() is then calling the on put event and when everything is correct commit() is called.\r\n\r\n```Post``` calls on post event and when this completes without any error commit() is called.\r\n\r\nAs you can see ```Post``` never calls on validate event.\r\nIn fact the on validate event is called in https://github.com/deployd/deployd/blob/master/lib/resources/collection/index.js#L638 when the script exists.\r\n\r\nWouldn't it be cleaner to call the on validate event also in the post() method (like here in put() https://github.com/deployd/deployd/blob/master/lib/resources/collection/index.js#L582)  and remove it here https://github.com/deployd/deployd/blob/master/lib/resources/collection/index.js#L638 ?\r\n\r\nThe second conditional branch (https://github.com/deployd/deployd/blob/master/lib/resources/collection/index.js#L635) could be removed (because validate is called in post()). \r\n\r\n ```\r\n if (query.id) {\r\n    beforeRequestDomain.event = \"PUT\";\r\n    collection.doBeforeRequestEvent(ctx, beforeRequestDomain, function(err) {\r\n      if (err) return fn(err);\r\n      put();\r\n    });\r\n  } else {\r\n    collection.doBeforeRequestEvent(ctx, beforeRequestDomain, function(err) {\r\n      if (err) return fn(err);\r\n      post();\r\n    });\r\n  }\r\n```\r\n\r\ninstead of\r\n\r\n```\r\nif (query.id) {\r\n    beforeRequestDomain.event = \"PUT\";\r\n    collection.doBeforeRequestEvent(ctx, beforeRequestDomain, function(err) {\r\n      if (err) return fn(err);\r\n      put();\r\n    });\r\n  } else if (collection.shouldRunEvent(collection.events.Validate, ctx)) {\r\n    collection.doBeforeRequestEvent(ctx, beforeRequestDomain, function(err) {\r\n      if (err) return fn(err);\r\n      collection.events.Validate.run(ctx, domain, function (err) {\r\n        if(err || domain.hasErrors()) return done(err || errors);\r\n        post();\r\n      });\r\n    });\r\n  } else {\r\n    collection.doBeforeRequestEvent(ctx, beforeRequestDomain, function(err) {\r\n      if (err) return fn(err);\r\n      post();\r\n    });\r\n  }\r\n```\r\n\r\nAnother nice effect would be the following order is the same for ```Put``` and ```Post```:\r\n- collection.validate (afaik validation of isRequired field and such things)\r\n- on validate event\r\n- persist in db\r\n\r\nCurrently for ```Post``` it's:\r\n- on validate event\r\n- collection.validate\r\n- persist in db\r\n\r\nAny thoughts about this? I hope my description is understandable."},"comment":{"url":"https://api.github.com/repos/deployd/deployd/issues/comments/256321550","html_url":"https://github.com/deployd/deployd/issues/769#issuecomment-256321550","issue_url":"https://api.github.com/repos/deployd/deployd/issues/769","id":256321550,"user":{"login":"andreialecu","id":697707,"avatar_url":"https://avatars.githubusercontent.com/u/697707?v=3","gravatar_id":"","url":"https://api.github.com/users/andreialecu","html_url":"https://github.com/andreialecu","followers_url":"https://api.github.com/users/andreialecu/followers","following_url":"https://api.github.com/users/andreialecu/following{/other_user}","gists_url":"https://api.github.com/users/andreialecu/gists{/gist_id}","starred_url":"https://api.github.com/users/andreialecu/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andreialecu/subscriptions","organizations_url":"https://api.github.com/users/andreialecu/orgs","repos_url":"https://api.github.com/users/andreialecu/repos","events_url":"https://api.github.com/users/andreialecu/events{/privacy}","received_events_url":"https://api.github.com/users/andreialecu/received_events","type":"User","site_admin":false},"created_at":"2016-10-26T11:34:00Z","updated_at":"2016-10-26T11:34:00Z","body":"Ah I see what you mean now. Doesn't necessarily seem like a problem, but feel free to make a PR for this and I will review it."}}
{"actor":{"display_login":"andreialecu"}}