)]}'
{"bgpvpn_dashboard/dashboards/project/bgpvpn/forms.py":[{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"eecc4b2f3de366f82d910479daa444027f6589b8","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"    def clean(self):"},{"line_number":48,"context_line":"        cleaned_data \u003d super(CommonData, self).clean()"},{"line_number":49,"context_line":"        if self.request.user.is_superuser:"},{"line_number":50,"context_line":"            for attribute in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":51,"context_line":"                if not cleaned_data.get(attribute):"},{"line_number":52,"context_line":"                    del cleaned_data[attribute]"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f1d235d_235a98f2","line":49,"range":{"start_line":49,"start_character":8,"end_line":49,"end_character":42},"updated":"2017-07-11 16:09:55.000000000","message":"IMHO it is better to use the policy check framework rather than referring to is_superuser.","commit_id":"bb004506bf1a7a4cc05ca503121b80e691353976"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"7b54808f9b9332be2c169611495071273e0f2211","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"    def clean(self):"},{"line_number":48,"context_line":"        cleaned_data \u003d super(CommonData, self).clean()"},{"line_number":49,"context_line":"        if self.request.user.is_superuser:"},{"line_number":50,"context_line":"            for attribute in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":51,"context_line":"                if not cleaned_data.get(attribute):"},{"line_number":52,"context_line":"                    del cleaned_data[attribute]"},{"line_number":53,"context_line":"                else:"},{"line_number":54,"context_line":"                    cleaned_data[attribute] \u003d bgpvpn_common.format_rt("},{"line_number":55,"context_line":"                        cleaned_data[attribute])"},{"line_number":56,"context_line":"        return cleaned_data"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    @staticmethod"},{"line_number":59,"context_line":"    def _del_attributes(attributes, data):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f1d235d_134b9f87","line":56,"range":{"start_line":49,"start_character":9,"end_line":56,"end_character":8},"updated":"2017-07-10 10:28:44.000000000","message":"It\u0027s better to change behavior in a different change and explain the reason there.","commit_id":"bb004506bf1a7a4cc05ca503121b80e691353976"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"eecc4b2f3de366f82d910479daa444027f6589b8","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"    def clean(self):"},{"line_number":48,"context_line":"        cleaned_data \u003d super(CommonData, self).clean()"},{"line_number":49,"context_line":"        if self.request.user.is_superuser:"},{"line_number":50,"context_line":"            for attribute in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":51,"context_line":"                if not cleaned_data.get(attribute):"},{"line_number":52,"context_line":"                    del cleaned_data[attribute]"},{"line_number":53,"context_line":"                else:"},{"line_number":54,"context_line":"                    cleaned_data[attribute] \u003d bgpvpn_common.format_rt("},{"line_number":55,"context_line":"                        cleaned_data[attribute])"},{"line_number":56,"context_line":"        return cleaned_data"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    @staticmethod"},{"line_number":59,"context_line":"    def _del_attributes(attributes, data):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f1d235d_a30228b3","line":56,"range":{"start_line":49,"start_character":9,"end_line":56,"end_character":8},"in_reply_to":"3f1d235d_134b9f87","updated":"2017-07-11 16:09:55.000000000","message":"+1","commit_id":"bb004506bf1a7a4cc05ca503121b80e691353976"}],"bgpvpn_dashboard/test/admin/test_tables.py":[{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"7b54808f9b9332be2c169611495071273e0f2211","unresolved":false,"context_lines":[{"line_number":30,"context_line":"                                                              \"id\")"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"class TestFormatRts(testtools.TestCase):"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"    def _mock_bgpvpn(self, bgpvpn):"},{"line_number":36,"context_line":"        return bgpvpn_api.Bgpvpn(bgpvpn)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def test_get_route_targets(self):"},{"line_number":39,"context_line":"        bgpvpn \u003d self._mock_bgpvpn({\"route_targets\": [\"65432:1\", \"65432:3\"]})"},{"line_number":40,"context_line":"        expected_result \u003d \"65432:1, 65432:3\""},{"line_number":41,"context_line":"        result \u003d bgpvpn_tables.get_route_targets(bgpvpn)"},{"line_number":42,"context_line":"        self.assertEqual(result, expected_result)"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def test_get_import_targets(self):"},{"line_number":45,"context_line":"        bgpvpn \u003d self._mock_bgpvpn({\"import_targets\": [\"65432:1\", \"65432:3\"]})"},{"line_number":46,"context_line":"        expected_result \u003d \"65432:1, 65432:3\""},{"line_number":47,"context_line":"        result \u003d bgpvpn_tables.get_import_targets(bgpvpn)"},{"line_number":48,"context_line":"        self.assertEqual(result, expected_result)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"    def test_get_export_targets(self):"},{"line_number":51,"context_line":"        bgpvpn \u003d self._mock_bgpvpn({\"export_targets\": [\"65432:1\", \"65432:3\"]})"},{"line_number":52,"context_line":"        expected_result \u003d \"65432:1, 65432:3\""},{"line_number":53,"context_line":"        result \u003d bgpvpn_tables.get_export_targets(bgpvpn)"},{"line_number":54,"context_line":"        self.assertEqual(result, expected_result)"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"class TestFunctionGet(testtools.TestCase):"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"    @mock.patch.object(bgpvpn_tables, \u0027reverse\u0027)"},{"line_number":60,"context_line":"    def test_get_network_url(self, mock_reverse):"},{"line_number":61,"context_line":"        mock_reverse.return_value \u003d \u0027foo_reverse_url\u0027"},{"line_number":62,"context_line":"        mock_network \u003d mock.Mock(id\u003d\u0027foo-id\u0027, name_or_id\u003d\"foo\")"},{"line_number":63,"context_line":"        result \u003d bgpvpn_tables.get_network_url(mock_network)"},{"line_number":64,"context_line":"        mock_reverse.assert_called_once_with("},{"line_number":65,"context_line":"            \u0027horizon:admin:networks:detail\u0027, args\u003d[\u0027foo-id\u0027])"},{"line_number":66,"context_line":"        self.assertEqual(\u0027\u003ca href\u003dfoo_reverse_url\u003efoo\u003c/a\u003e\u0027, result)"},{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    @mock.patch.object(bgpvpn_tables, \u0027reverse\u0027)"},{"line_number":69,"context_line":"    def test_get_router_url(self, mock_reverse):"},{"line_number":70,"context_line":"        mock_reverse.return_value \u003d \u0027foo_reverse_url\u0027"},{"line_number":71,"context_line":"        mock_network \u003d mock.Mock(id\u003d\u0027foo-id\u0027, name_or_id\u003d\"foo\")"},{"line_number":72,"context_line":"        result \u003d bgpvpn_tables.get_router_url(mock_network)"},{"line_number":73,"context_line":"        mock_reverse.assert_called_once_with("},{"line_number":74,"context_line":"            \u0027horizon:admin:routers:detail\u0027, args\u003d[\u0027foo-id\u0027])"},{"line_number":75,"context_line":"        self.assertEqual(\u0027\u003ca href\u003dfoo_reverse_url\u003efoo\u003c/a\u003e\u0027, result)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f1d235d_53c47740","line":73,"range":{"start_line":33,"start_character":0,"end_line":73,"end_character":45},"updated":"2017-07-10 10:28:44.000000000","message":"This is quite a lot of code, compared to the length/complexity of the code being tested.\n\nI\u0027ll merge, because this is correct, but I think that we need to think about the cost/benefit of adding this kind of test.","commit_id":"bb004506bf1a7a4cc05ca503121b80e691353976"}],"bgpvpn_dashboard/test/admin/test_views.py":[{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"7b54808f9b9332be2c169611495071273e0f2211","unresolved":false,"context_lines":[{"line_number":49,"context_line":"            }"},{"line_number":50,"context_line":"        return bgpvpn_api.Bgpvpn(bgpvpn_info)"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    @mock.patch.object(bgpvpn_views.api, \u0027keystone\u0027, autospec\u003dTrue)"},{"line_number":53,"context_line":"    def test_get_tenant_name(self, mock_api):"},{"line_number":54,"context_line":"        Tenant \u003d namedtuple(\"Tenant\", [\"id\", \"name\"])"},{"line_number":55,"context_line":"        tenant_name \u003d Tenant(\"tenant_id\", \"tenant_name\")"},{"line_number":56,"context_line":"        expected_result \u003d {\"tenant_id\": \"tenant_name\"}"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"        mock_api.tenant_list.return_value \u003d [tenant_name], False"},{"line_number":59,"context_line":"        result \u003d self.bgpvpn_view._get_tenant_name()"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"        mock_api.tenant_list.assert_called_once_with(self.bgpvpn_view.request)"},{"line_number":62,"context_line":"        self.assertEqual(result, expected_result)"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    @mock.patch(\u0027%s.IndexView._get_tenant_name\u0027 % VIEWS,"},{"line_number":65,"context_line":"                return_value\u003d{\"tenant_id\": \"tenant_name\"})"},{"line_number":66,"context_line":"    @mock.patch.object(bgpvpn_views, \u0027api\u0027, autospec\u003dTrue)"},{"line_number":67,"context_line":"    @mock.patch.object(bgpvpn_views, \u0027bgpvpn_api\u0027, autospec\u003dTrue)"},{"line_number":68,"context_line":"    def test_get_data(self, mock_bgpvpn_api, mock_api, mock_get_tenant_name):"},{"line_number":69,"context_line":"        bgpvpn_foo \u003d self._get_mock_bgpvpn(\"foo\")"},{"line_number":70,"context_line":"        bgpvpn_bar \u003d self._get_mock_bgpvpn(\"bar\")"},{"line_number":71,"context_line":"        mock_neutron_client \u003d mock_api.neutron.neutronclient(mock.Mock())"},{"line_number":72,"context_line":"        mock_bgpvpn_api.bgpvpns_list.return_value \u003d [bgpvpn_foo, bgpvpn_bar]"},{"line_number":73,"context_line":"        mock_neutron_client.list_networks.return_value \u003d []"},{"line_number":74,"context_line":"        mock_neutron_client.list_routers.return_value \u003d []"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"        expected_bgpvpns \u003d [bgpvpn_foo, bgpvpn_bar]"},{"line_number":77,"context_line":"        result \u003d self.bgpvpn_view.get_data()"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"        mock_get_tenant_name.assert_called_once_with()"},{"line_number":80,"context_line":"        self.assertEqual(result, expected_bgpvpns)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f1d235d_73ad3330","line":80,"range":{"start_line":52,"start_character":0,"end_line":80,"end_character":50},"updated":"2017-07-10 10:28:44.000000000","message":"Looking at these two tests, I read again the related code, to conclude that there are possibly problematic/inefficient cases:\n- retrieving all tenants while only some of them may have BGPVPNs may be inefficient\n  (btw, why adding memoize to a function called only once ?)\n- retrieving all tenants, as currently done, may fail (case where has_more is True)\n- retrieving all routers while only some of them may have BGPVPNs may be inefficient\n- retrieving all networks while only some of them may have BGPVPNs may be inefficient\n\nI think that it\u0027s worth refactoring the code before merging any test.\n\nI would suggest:\n* calling api.neutron.router_get(self.request, router.id) inside the loop\n* calling api.neutron.network_get(self.request, network.id) inside the loop\n* adding a memoized _get_tenant_name(self, tenant_id) and calling it inside the loop","commit_id":"bb004506bf1a7a4cc05ca503121b80e691353976"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"eecc4b2f3de366f82d910479daa444027f6589b8","unresolved":false,"context_lines":[{"line_number":49,"context_line":"            }"},{"line_number":50,"context_line":"        return bgpvpn_api.Bgpvpn(bgpvpn_info)"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    @mock.patch.object(bgpvpn_views.api, \u0027keystone\u0027, autospec\u003dTrue)"},{"line_number":53,"context_line":"    def test_get_tenant_name(self, mock_api):"},{"line_number":54,"context_line":"        Tenant \u003d namedtuple(\"Tenant\", [\"id\", \"name\"])"},{"line_number":55,"context_line":"        tenant_name \u003d Tenant(\"tenant_id\", \"tenant_name\")"},{"line_number":56,"context_line":"        expected_result \u003d {\"tenant_id\": \"tenant_name\"}"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"        mock_api.tenant_list.return_value \u003d [tenant_name], False"},{"line_number":59,"context_line":"        result \u003d self.bgpvpn_view._get_tenant_name()"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"        mock_api.tenant_list.assert_called_once_with(self.bgpvpn_view.request)"},{"line_number":62,"context_line":"        self.assertEqual(result, expected_result)"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    @mock.patch(\u0027%s.IndexView._get_tenant_name\u0027 % VIEWS,"},{"line_number":65,"context_line":"                return_value\u003d{\"tenant_id\": \"tenant_name\"})"},{"line_number":66,"context_line":"    @mock.patch.object(bgpvpn_views, \u0027api\u0027, autospec\u003dTrue)"},{"line_number":67,"context_line":"    @mock.patch.object(bgpvpn_views, \u0027bgpvpn_api\u0027, autospec\u003dTrue)"},{"line_number":68,"context_line":"    def test_get_data(self, mock_bgpvpn_api, mock_api, mock_get_tenant_name):"},{"line_number":69,"context_line":"        bgpvpn_foo \u003d self._get_mock_bgpvpn(\"foo\")"},{"line_number":70,"context_line":"        bgpvpn_bar \u003d self._get_mock_bgpvpn(\"bar\")"},{"line_number":71,"context_line":"        mock_neutron_client \u003d mock_api.neutron.neutronclient(mock.Mock())"},{"line_number":72,"context_line":"        mock_bgpvpn_api.bgpvpns_list.return_value \u003d [bgpvpn_foo, bgpvpn_bar]"},{"line_number":73,"context_line":"        mock_neutron_client.list_networks.return_value \u003d []"},{"line_number":74,"context_line":"        mock_neutron_client.list_routers.return_value \u003d []"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"        expected_bgpvpns \u003d [bgpvpn_foo, bgpvpn_bar]"},{"line_number":77,"context_line":"        result \u003d self.bgpvpn_view.get_data()"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"        mock_get_tenant_name.assert_called_once_with()"},{"line_number":80,"context_line":"        self.assertEqual(result, expected_bgpvpns)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f1d235d_23e738e7","line":80,"range":{"start_line":52,"start_character":0,"end_line":80,"end_character":50},"in_reply_to":"3f1d235d_73ad3330","updated":"2017-07-11 16:09:55.000000000","message":"I tend to agree with Thomas. In horizon side, we try to retrieve information only for required information by passing appropriate filter values. I think the similar things can be done in the BGPVPN dashboard.","commit_id":"bb004506bf1a7a4cc05ca503121b80e691353976"}]}