)]}'
{"bgpvpn_dashboard/dashboards/project/bgpvpn/forms.py":[{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"52b197ab11493596dc3cd1c3b3f078ff00bf9cf6","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_3ea27afe","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"updated":"2017-09-15 12:31:16.000000000","message":"If one of the RT attributes is set to an empty string, then the test for data.get(key) will be False, and the attribute will be kept in params as an empty string, instead of as an empty list.\n\nTo avoid this, a solution would be to: always remove the attribute from data, and have bgpvpn_common.format_rt be changed to return an empty list when called with None or \u0027\u0027.\n\nI\u0027m unsure, when looking at the code, if its possible to have an empty string come up here (the REGEX does not allow an empty string, but the field is required\u003dFalse, which is I guess what allows to reset the field?), but wouldn\u0027t it be better to have this function be robust ?","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"8da79f33e45a17c379e49f978df8f6ea032f333d","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_81058e13","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"5f2577a7_390df4a0","updated":"2017-09-18 08:44:10.000000000","message":"why not with pop ?\ndon\u0027t we have to handle the case where key is not in data ?","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":21800,"name":"Cedric Savignan","email":"cedric.savignan@orange.com","username":"csavignan"},"change_message_id":"b47da90eda5caf8e66ce216c70d5f25365cb7d96","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_de57f657","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"5f2577a7_3ea27afe","updated":"2017-09-15 13:09:10.000000000","message":"Yes. I will remove the test for data.get(key) to allow have a list even if the field is empty.\n\nThe REGEX allow empty string but check in admin/forms.py the variable named RTRD_REGEX in line 29.\n\nRTRD_REGEX \u003d constants.RTRD_REGEX[4:-1]\n\nI don\u0027t use the first part of this REGEX which allow empty string.","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":21800,"name":"Cedric Savignan","email":"cedric.savignan@orange.com","username":"csavignan"},"change_message_id":"668c26f6ab7dc219ce808d2ef0a70317773f6433","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff5d4b2f_bfd96b23","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"5f2577a7_81058e13","updated":"2017-09-19 09:00:15.000000000","message":"The case where key isn\u0027t in data should never be possible.\nIt can occur when a RT is invalid but the validation of form should stop before when the verification of the RT is done with the REGEX.\nEven so we can use pop to be more secure.","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"9985b66af35a222b3f65b50e6ed8a213ebe3f032","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_b9968492","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"5f2577a7_9933c8f6","updated":"2017-09-15 13:42:50.000000000","message":"Another possible way:\n\n  params[key] \u003d bgpvpn_common.format_rt(data.pop(key, None))\n\nAnd modify format_rt :\n\n    if route_targets:\n        return re.compile(\" *, *\").split(route_targets)\n    else:\n        return []","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":21800,"name":"Cedric Savignan","email":"cedric.savignan@orange.com","username":"csavignan"},"change_message_id":"0a7741f0bee06bd6f5b0de428fd585ad2042a948","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_9933c8f6","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"5f2577a7_9e7e0ead","updated":"2017-09-15 13:35:52.000000000","message":"If the field is empty the form submission is still allowed. This attribute is empty in data. For exemple : {\u0027type\u0027: u\u0027l3\u0027, \u0027export_targets\u0027: u\u0027\u0027 ...\n\nOK. I\u0027ve just seen. I should set a empty list if the field is empty.\n\n            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:\n                if data.get(key):\n                    params[key] \u003d bgpvpn_common.format_rt(data[key])\n                else:\n                    params[key] \u003d []\n                del data[key]","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":21800,"name":"Cedric Savignan","email":"cedric.savignan@orange.com","username":"csavignan"},"change_message_id":"b7166462d6b172e1ba39877d2a75ee5388a69014","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_390df4a0","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"5f2577a7_b9968492","updated":"2017-09-15 14:01:17.000000000","message":"Done","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"576e5dbfec5b8f9f37fdd8edb8b645d391958ec3","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_9e7e0ead","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"5f2577a7_de57f657","updated":"2017-09-15 13:18:47.000000000","message":"Yes, I had a look at this earlier, which lead me to file this \u0027future\u0027 bug: https://bugs.launchpad.net/bgpvpn/+bug/1717512\n\nOne question though: what happens when the user does not put anything in the field ?  Is it the required\u003dFalse that results in still allowing the form submission  ? What does \u0027data\u0027 contains for this field (None?) ?","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":21800,"name":"Cedric Savignan","email":"cedric.savignan@orange.com","username":"csavignan"},"change_message_id":"64edeb1a901aaa3f1c9bc347aa48f0674ea57e24","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f5c4f37_640da38f","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"ff5d4b2f_5456d108","updated":"2017-09-21 07:22:06.000000000","message":"OK. So I don\u0027t touch anymore this part?","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"},{"author":{"_account_id":12021,"name":"Thomas Morin","email":"tmmorin.orange@gmail.com","username":"tmorin"},"change_message_id":"2eb654ae63f46bfc83d74bc0648644c797fff121","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        params \u003d {}"},{"line_number":75,"context_line":"        if request.user.is_superuser:"},{"line_number":76,"context_line":"            for key in bgpvpn_common.RT_FORMAT_ATTRIBUTES:"},{"line_number":77,"context_line":"                if data.get(key):"},{"line_number":78,"context_line":"                    params[key] \u003d bgpvpn_common.format_rt(data[key])"},{"line_number":79,"context_line":"                    del data[key]"},{"line_number":80,"context_line":"        params.update(data)"},{"line_number":81,"context_line":"        try:"},{"line_number":82,"context_line":"            if self.action \u003d\u003d \u0027update\u0027:"},{"line_number":83,"context_line":"                if request.user.is_superuser and data.get(\u0027tenant_id\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff5d4b2f_5456d108","line":80,"range":{"start_line":77,"start_character":0,"end_line":80,"end_character":27},"in_reply_to":"ff5d4b2f_bfd96b23","updated":"2017-09-19 10:17:24.000000000","message":"You are correct that the case for \u0027key not in data\u0027 is avoided.\n\nThe question which I should have asked is: shouldn\u0027t we handle the case where data[key] is None ?  in this case, with the code above, params will contain None for this key, instead of [], which defeats the change in format_rt...\n\nOverall, the end result is similar, since the API accepts both [] and None...\n\nI still have a preference for pop here, making it clear that the field is always reformated before passing to API.","commit_id":"3204b83ae9110179c5293628f7e7e6c59c32a2eb"}]}