)]}'
{"glance/api/v2/image_members.py":[{"author":{"_account_id":8001,"name":"John Warren","email":"jswarren@us.ibm.com","username":"jswarren"},"change_message_id":"cf7df8ad125cd7ad435c4024c1e2fb450f01b515","unresolved":false,"context_lines":[{"line_number":66,"context_line":""},{"line_number":67,"context_line":"            for member in member_repo.list():"},{"line_number":68,"context_line":"                if member.member_id \u003d\u003d member_id:"},{"line_number":69,"context_line":"                    msg \u003d _(\u0027The target member %(member_id)s has been \u0027"},{"line_number":70,"context_line":"                            \u0027associated with image %(image_id)s.\u0027 %"},{"line_number":71,"context_line":"                            dict(member_id\u003dmember_id,"},{"line_number":72,"context_line":"                                 image_id\u003dimage_id))"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAS3%2F%2FbXE%3D","line":69,"updated":"2013-09-26 20:50:28.000000000","message":"Would it make sense to say \"...is already associated with...\", to clarify that the error happened because the request is redundant?  The current message is a response you would expect if the operation succeeded.","commit_id":"7415cef5dd79dba39c492efc60ae0d2ba14e59cf"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"4e1057ed84fc75383199f2e4295ebaa6a084d050","unresolved":false,"context_lines":[{"line_number":66,"context_line":""},{"line_number":67,"context_line":"            for member in member_repo.list():"},{"line_number":68,"context_line":"                if member.member_id \u003d\u003d member_id:"},{"line_number":69,"context_line":"                    msg \u003d _(\u0027The target member %(member_id)s has been \u0027"},{"line_number":70,"context_line":"                            \u0027associated with image %(image_id)s.\u0027 %"},{"line_number":71,"context_line":"                            dict(member_id\u003dmember_id,"},{"line_number":72,"context_line":"                                 image_id\u003dimage_id))"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAS3%2F%2Fa6I%3D","line":69,"in_reply_to":"AAAAS3%2F%2FbPo%3D","updated":"2013-09-27 02:06:56.000000000","message":"Done. Thanks for the comments, Mark. It makes more sense to move the logic into domain.","commit_id":"7415cef5dd79dba39c492efc60ae0d2ba14e59cf"},{"author":{"_account_id":616,"name":"Mark Washenberger","email":"mark.washenberger@markwash.net","username":"markwash"},"change_message_id":"f8592ff7903b26b50050d4998b58c64654c12358","unresolved":false,"context_lines":[{"line_number":66,"context_line":""},{"line_number":67,"context_line":"            for member in member_repo.list():"},{"line_number":68,"context_line":"                if member.member_id \u003d\u003d member_id:"},{"line_number":69,"context_line":"                    msg \u003d _(\u0027The target member %(member_id)s has been \u0027"},{"line_number":70,"context_line":"                            \u0027associated with image %(image_id)s.\u0027 %"},{"line_number":71,"context_line":"                            dict(member_id\u003dmember_id,"},{"line_number":72,"context_line":"                                 image_id\u003dimage_id))"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAS3%2F%2FbPo%3D","line":69,"in_reply_to":"AAAAS3%2F%2FbXE%3D","updated":"2013-09-26 21:48:42.000000000","message":"When you fix this error message, can you also add a note to move this logic down into the domain as a TODO?","commit_id":"7415cef5dd79dba39c492efc60ae0d2ba14e59cf"},{"author":{"_account_id":616,"name":"Mark Washenberger","email":"mark.washenberger@markwash.net","username":"markwash"},"change_message_id":"34b6f0c86090ec4c48488c62daf9e8eb2dad0fb3","unresolved":false,"context_lines":[{"line_number":65,"context_line":"            member_repo \u003d image.get_member_repo()"},{"line_number":66,"context_line":"            new_member \u003d image_member_factory.new_image_member(image,"},{"line_number":67,"context_line":"                                                               member_id)"},{"line_number":68,"context_line":"            member \u003d member_repo.add(new_member)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"            return member"},{"line_number":71,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAS3%2F%2FYcc%3D","line":68,"updated":"2013-09-27 18:35:48.000000000","message":"I like the change in this file, but it feels better to me to change the domain so that uniqueness is enforced during member_repo.add(...) rather than image_member_factory.new_image_member(...)","commit_id":"0a88e9f191d9db3df23d9143b335baf94a30ac5e"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"792b074e58923b01ce911b5aee9f7a3e167c52d2","unresolved":false,"context_lines":[{"line_number":65,"context_line":"            member_repo \u003d image.get_member_repo()"},{"line_number":66,"context_line":"            new_member \u003d image_member_factory.new_image_member(image,"},{"line_number":67,"context_line":"                                                               member_id)"},{"line_number":68,"context_line":"            member \u003d member_repo.add(new_member)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"            return member"},{"line_number":71,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAS3%2F%2FW34%3D","line":68,"in_reply_to":"AAAAS3%2F%2FYcc%3D","updated":"2013-09-28 15:03:37.000000000","message":"Thanks for the comments, Mark. But I would like to argue :-) Actually, I have considered about placing the checking into member_repo.add(...) or image_member_factory.new_image_member(...) before I submitted the fix. Let me show my thoughts.\n\n1. Based on my understanding, the image member factory should be responsible for the member\u0027s validity. For example, just like Papa Johns must be sure the pizza is good before delivering to customer, since it\u0027s the \"factory\" of the pizza. And the add function of member repo is like the pizza delivering process. In other words, the pizza quality assurance is not the responsibility of the pizza delivery man :) \n\n2. It would be a question where to place the check if we want to place the checking into member_repo.add(...). Since we don\u0027t want to see user is told the member is duplicated when the member is passed down to the member repo of db. So seems the best place is placing the checking into the member repo which is first repo on the responsibility chain. Now it\u0027s the member repo of authorization.py. However, as you know, the gate keeper on the responsibility chain should be focusing what he should do instead of doing this kind of validity checking. And if there is a new 1st gate keeper, we may need update the code again.","commit_id":"0a88e9f191d9db3df23d9143b335baf94a30ac5e"},{"author":{"_account_id":4463,"name":"iccha-sethi","email":"iccha.sethi@rackspace.com","username":"iccha-sethi"},"change_message_id":"a89f30068477f1435fc19d375cc6fd6a01f59dbd","unresolved":false,"context_lines":[{"line_number":73,"context_line":"        except exception.Forbidden as e:"},{"line_number":74,"context_line":"            raise webob.exc.HTTPForbidden(explanation\u003dunicode(e))"},{"line_number":75,"context_line":"        except exception.Duplicate as e:"},{"line_number":76,"context_line":"            raise webob.exc.HTTPBadRequest(explanation\u003dunicode(e))"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"    @utils.mutating"},{"line_number":79,"context_line":"    def update(self, req, image_id, member_id, status):"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAS3%2F%2FT9I%3D","line":76,"updated":"2013-09-30 13:39:00.000000000","message":"Do you think BadRequest is the right return code?","commit_id":"1683cd03248f2fdb840b4b788282f682e472e98e"},{"author":{"_account_id":7703,"name":"Venkatesh Sampath","email":"venkatesh.sampath@outlook.com","username":"venkateshsampath"},"change_message_id":"b602cc54a78e27e1ca20a7fb1735ea0bcf4139b4","unresolved":false,"context_lines":[{"line_number":73,"context_line":"        except exception.Forbidden as e:"},{"line_number":74,"context_line":"            raise webob.exc.HTTPForbidden(explanation\u003dunicode(e))"},{"line_number":75,"context_line":"        except exception.Duplicate as e:"},{"line_number":76,"context_line":"            raise webob.exc.HTTPBadRequest(explanation\u003dunicode(e))"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"    @utils.mutating"},{"line_number":79,"context_line":"    def update(self, req, image_id, member_id, status):"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAS3%2F%2FT8I%3D","line":76,"updated":"2013-09-30 13:46:16.000000000","message":"Hi Fei,\nMay be 409 - HTTPConflict could be substituted for 400? HTTPBadRequest doesn\u0027t sound like the right exception in this context.","commit_id":"1683cd03248f2fdb840b4b788282f682e472e98e"}],"glance/domain/__init__.py":[{"author":{"_account_id":4463,"name":"iccha-sethi","email":"iccha.sethi@rackspace.com","username":"iccha-sethi"},"change_message_id":"24187d9992b6eac205539985b6e8e7ef18cab28e","unresolved":false,"context_lines":[{"line_number":211,"context_line":""},{"line_number":212,"context_line":"    def new_image_member(self, image, member_id):"},{"line_number":213,"context_line":"        member_repo \u003d image.get_member_repo()"},{"line_number":214,"context_line":"        for member in member_repo.list():"},{"line_number":215,"context_line":"            if member.member_id \u003d\u003d member_id:"},{"line_number":216,"context_line":"                msg \u003d _(\u0027The target member %(member_id)s is already \u0027"},{"line_number":217,"context_line":"                        \u0027associated with image %(image_id)s.\u0027 %"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FZ0U%3D","line":214,"updated":"2013-09-27 12:35:22.000000000","message":"Instead of list and looping through we could also do a get and see if exception not found and then add it?","commit_id":"d40176eda6d564e531c5cc8a61c94d6311dacf6b"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"1c94c57dfcd9545566cf63b111a2ce2ffbe34c12","unresolved":false,"context_lines":[{"line_number":211,"context_line":""},{"line_number":212,"context_line":"    def new_image_member(self, image, member_id):"},{"line_number":213,"context_line":"        member_repo \u003d image.get_member_repo()"},{"line_number":214,"context_line":"        for member in member_repo.list():"},{"line_number":215,"context_line":"            if member.member_id \u003d\u003d member_id:"},{"line_number":216,"context_line":"                msg \u003d _(\u0027The target member %(member_id)s is already \u0027"},{"line_number":217,"context_line":"                        \u0027associated with image %(image_id)s.\u0027 %"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FZI4%3D","line":214,"in_reply_to":"AAAAS3%2F%2FZ0U%3D","updated":"2013-09-27 16:00:01.000000000","message":"Seems it\u0027s a good idea. Thanks.\nDone.","commit_id":"d40176eda6d564e531c5cc8a61c94d6311dacf6b"},{"author":{"_account_id":4463,"name":"iccha-sethi","email":"iccha.sethi@rackspace.com","username":"iccha-sethi"},"change_message_id":"24187d9992b6eac205539985b6e8e7ef18cab28e","unresolved":false,"context_lines":[{"line_number":217,"context_line":"                        \u0027associated with image %(image_id)s.\u0027 %"},{"line_number":218,"context_line":"                        dict(member_id\u003dmember_id,"},{"line_number":219,"context_line":"                             image_id\u003dimage.image_id))"},{"line_number":220,"context_line":"                raise exception.Duplicate(msg)"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"        created_at \u003d timeutils.utcnow()"},{"line_number":223,"context_line":"        updated_at \u003d created_at"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FZ0Q%3D","line":220,"updated":"2013-09-27 12:35:22.000000000","message":"Also can you add domain tests for this?","commit_id":"d40176eda6d564e531c5cc8a61c94d6311dacf6b"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"1c94c57dfcd9545566cf63b111a2ce2ffbe34c12","unresolved":false,"context_lines":[{"line_number":217,"context_line":"                        \u0027associated with image %(image_id)s.\u0027 %"},{"line_number":218,"context_line":"                        dict(member_id\u003dmember_id,"},{"line_number":219,"context_line":"                             image_id\u003dimage.image_id))"},{"line_number":220,"context_line":"                raise exception.Duplicate(msg)"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"        created_at \u003d timeutils.utcnow()"},{"line_number":223,"context_line":"        updated_at \u003d created_at"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAS3%2F%2FZI0%3D","line":220,"in_reply_to":"AAAAS3%2F%2FZ0Q%3D","updated":"2013-09-27 16:00:01.000000000","message":"Done","commit_id":"d40176eda6d564e531c5cc8a61c94d6311dacf6b"}],"glance/tests/unit/test_domain.py":[{"author":{"_account_id":4463,"name":"iccha-sethi","email":"iccha.sethi@rackspace.com","username":"iccha-sethi"},"change_message_id":"91afdc1a272dbf2311f0ed3bc11a9d80fa16b695","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        self.member \u003d member"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def get(self, member_id):"},{"line_number":30,"context_line":"        if self.member:"},{"line_number":31,"context_line":"            return self.member"},{"line_number":32,"context_line":"        else:"},{"line_number":33,"context_line":"            raise exception.NotFound()"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FY8M%3D","line":30,"updated":"2013-09-27 16:34:25.000000000","message":"may be do a self.member.member_id \u003d\u003d member_id?","commit_id":"5097c6cfd31cdf081ca06d64af9257339bd6ddb4"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"fabd5578dd40c27ded58e3aa42b972a9b241c923","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        self.member \u003d member"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def get(self, member_id):"},{"line_number":30,"context_line":"        if self.member:"},{"line_number":31,"context_line":"            return self.member"},{"line_number":32,"context_line":"        else:"},{"line_number":33,"context_line":"            raise exception.NotFound()"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAS3%2F%2FY2g%3D","line":30,"in_reply_to":"AAAAS3%2F%2FY8M%3D","updated":"2013-09-27 16:55:24.000000000","message":"I would say it\u0027s ok. Since only the specific member will be passed in. So it means we can control the logic by passing in None or a real member during create the instance of FakeImageMemberRepo. But sure, it would be more clear to add the member_id checking. I will submit a new ps soon. Thanks, iccha.","commit_id":"5097c6cfd31cdf081ca06d64af9257339bd6ddb4"}]}