{"action":"created","issue":{"url":"https://api.github.com/repos/keystonejs/keystone/issues/1194","labels_url":"https://api.github.com/repos/keystonejs/keystone/issues/1194/labels{/name}","comments_url":"https://api.github.com/repos/keystonejs/keystone/issues/1194/comments","events_url":"https://api.github.com/repos/keystonejs/keystone/issues/1194/events","html_url":"https://github.com/keystonejs/keystone/issues/1194","id":60629076,"number":1194,"title":"UpdateHandler refactoring","user":{"login":"creynders","id":436347,"avatar_url":"https://avatars.githubusercontent.com/u/436347?v=3","gravatar_id":"","url":"https://api.github.com/users/creynders","html_url":"https://github.com/creynders","followers_url":"https://api.github.com/users/creynders/followers","following_url":"https://api.github.com/users/creynders/following{/other_user}","gists_url":"https://api.github.com/users/creynders/gists{/gist_id}","starred_url":"https://api.github.com/users/creynders/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/creynders/subscriptions","organizations_url":"https://api.github.com/users/creynders/orgs","repos_url":"https://api.github.com/users/creynders/repos","events_url":"https://api.github.com/users/creynders/events{/privacy}","received_events_url":"https://api.github.com/users/creynders/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/keystonejs/keystone/labels/in%20progress","name":"in progress","color":"ededed"}],"state":"open","locked":false,"assignee":{"login":"creynders","id":436347,"avatar_url":"https://avatars.githubusercontent.com/u/436347?v=3","gravatar_id":"","url":"https://api.github.com/users/creynders","html_url":"https://github.com/creynders","followers_url":"https://api.github.com/users/creynders/followers","following_url":"https://api.github.com/users/creynders/following{/other_user}","gists_url":"https://api.github.com/users/creynders/gists{/gist_id}","starred_url":"https://api.github.com/users/creynders/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/creynders/subscriptions","organizations_url":"https://api.github.com/users/creynders/orgs","repos_url":"https://api.github.com/users/creynders/repos","events_url":"https://api.github.com/users/creynders/events{/privacy}","received_events_url":"https://api.github.com/users/creynders/received_events","type":"User","site_admin":false},"milestone":null,"comments":6,"created_at":"2015-03-11T09:10:36Z","updated_at":"2015-03-12T08:33:45Z","closed_at":null,"body":"Related to #1150 since we need to clean up `updateHandler` first before we can even try and get rid of the other switch-cases.\r\n\r\nSo, I've delved into the mysteries of the `UpdateHandler` and so far, AFAICT, its life-cycle is as follows:\r\n\r\nIt iterates over all fields and does inline type-specific validation for some field types.\r\nNext, it calls the `validateInput` method of the field.\r\nAnd lastly it calls field-specific validation, as declared in the models.\r\nAll of this sprinkled with conditionals in case the field value is already invalid.\r\n\r\nHowever, there's a number of problems:\r\n\r\n1. the inline type-specific validation _seems_ superfluous since field types already have a `validateInput` method, but I need to check whether the current correct functioning of item validation isn't an ephemeral side-effect of the inline validation and `validateInput` combined.\r\n1. `validateInput` is called twice for each field that has a changed (valid) value. This definitely needs to go.\r\n1. `UpdateHandler` is monolithic and could _definitely_ use some refactoring\r\n\t1. its constructor signature is messy and leaks responsibilities all over the place, for instance: `req` is a required argument, but the only use of `req` is for its `flash` method to warn the user of errors.\r\n\t1. `res` is even worse, it's a required argument, but isn't used. At all.\r\n\t1. `UpdateHandler#process` is huge and messy\r\n\r\nSo what I'll be aiming for is:\r\n\r\n1. drop the dependency on `req`\r\n1. drop the dependency on `res`\r\n1. consolidate the inline error handling into a separate hook/whatever\r\n1. drop inline validators and only use the type specific validators as declare by the types\r\n1. tests, duh!\r\n\r\nThese are breaking changes, no doubt, but well worth the effort and even easily patchable if backwards-compatibility is mandatory.\r\n\r\n@jedwatson let me know what you think.\r\n\r\nI'll turn this into a PR the moment i've got some non-explosive code."},"comment":{"url":"https://api.github.com/repos/keystonejs/keystone/issues/comments/78440828","html_url":"https://github.com/keystonejs/keystone/issues/1194#issuecomment-78440828","issue_url":"https://api.github.com/repos/keystonejs/keystone/issues/1194","id":78440828,"user":{"login":"creynders","id":436347,"avatar_url":"https://avatars.githubusercontent.com/u/436347?v=3","gravatar_id":"","url":"https://api.github.com/users/creynders","html_url":"https://github.com/creynders","followers_url":"https://api.github.com/users/creynders/followers","following_url":"https://api.github.com/users/creynders/following{/other_user}","gists_url":"https://api.github.com/users/creynders/gists{/gist_id}","starred_url":"https://api.github.com/users/creynders/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/creynders/subscriptions","organizations_url":"https://api.github.com/users/creynders/orgs","repos_url":"https://api.github.com/users/creynders/repos","events_url":"https://api.github.com/users/creynders/events{/privacy}","received_events_url":"https://api.github.com/users/creynders/received_events","type":"User","site_admin":false},"created_at":"2015-03-12T08:33:45Z","updated_at":"2015-03-12T08:33:45Z","body":"Full tracing output for a valid form submission:\r\n\r\n```\r\nGET /keystone/users/55013ef0c6c0d7da7b260fc0 200 536.937 ms\r\nUpdateHandler#process\r\nUpdateHandler#process field: name\r\nUpdateHandler#process inline validation: name\r\nUpdateHandler#process field.validateInput(): name\r\nUpdateHandler#process field.updateItem() name\r\nUpdateHandler#process field: email\r\nUpdateHandler#process inline validation: email\r\nUpdateHandler#process field.validateInput(): email\r\nUpdateHandler#process field.updateItem() email\r\nUpdateHandler#process field: brand\r\nUpdateHandler#process inline validation: brand\r\nUpdateHandler#process field.validateInput(): brand\r\nUpdateHandler#process field.updateItem() brand\r\nUpdateHandler#process field: avatar\r\nUpdateHandler#process inline validation: avatar\r\nUpdateHandler#process field.validateInput(): avatar\r\nUpdateHandler#process field.updateItem() avatar\r\nUpdateHandler#process field: password\r\nUpdateHandler#process inline validation: password\r\nUpdateHandler#process field: isAdmin\r\nUpdateHandler#process inline validation: isAdmin\r\nUpdateHandler#process field.validateInput(): isAdmin\r\nUpdateHandler#process field.updateItem() isAdmin\r\nUpdateHandler#progress\r\nUpdateHandler#progress remove request handler: avatar\r\nUpdateHandler#process getRequestHandler callback avatar\r\nUpdateHandler#progress\r\nUpdateHandler#progress remove request handler: brand\r\nUpdateHandler#process getRequestHandler callback brand\r\nUpdateHandler#progress\r\nUpdateHandler#saveItem\r\nUpdateHandler#saveItem callback\r\nPOST /keystone/users/55013ef0c6c0d7da7b260fc0 302 100.785 ms\r\nGET /keystone/users/55013ef0c6c0d7da7b260fc0 200 229.735 ms\r\n```"}}