From 30132e2578e4f943019d53190941e449a53cfc40 Mon Sep 17 00:00:00 2001 From: Valentin Deniaud Date: Tue, 28 Jan 2020 18:16:03 +0100 Subject: [PATCH] clicrdv: improve API error handling (#39273) --- passerelle/apps/clicrdv/models.py | 67 ++++++++++++++++--------------- tests/test_clicrdv.py | 24 ++++++++--- 2 files changed, 52 insertions(+), 39 deletions(-) diff --git a/passerelle/apps/clicrdv/models.py b/passerelle/apps/clicrdv/models.py index c781f18c..e0ef52dc 100644 --- a/passerelle/apps/clicrdv/models.py +++ b/passerelle/apps/clicrdv/models.py @@ -57,12 +57,26 @@ class ClicRdv(BaseResource): else: url = url + '?apikey=%s&format=json' % self.apikey basic_auth = requests.auth.HTTPBasicAuth(self.username, self.password) - return self.requests.request(method, url, auth=basic_auth, **kwargs) + response = self.requests.request(method, url, auth=basic_auth, **kwargs) + try: + response.raise_for_status() + except requests.exceptions.HTTPError as e: + try: + error = response.json() + except ValueError: + error = 'invalid json, got %s' % response.text + return { + 'success': False, + 'error': '%s : %s' % (e, error), + } + return response.json() @endpoint(name='interventionsets') def get_interventionsets(self, request, **kwargs): response = self.request('interventionsets') - records = response.json().get('records') + if 'error' in response: + return response + records = response.get('records', []) records.sort(lambda x,y: cmp(x['sort'], y['sort'])) ret = [] for record in records: @@ -74,7 +88,9 @@ class ClicRdv(BaseResource): def get_interventions(self, request, set, **kwargs): ret = [] response = self.request('interventions?interventionset_id=%s' % set) - records = response.json().get('records') + if 'error' in response: + return response + records = response.get('records', []) records.sort(lambda x,y: cmp(x['sort'], y['sort'])) for record in records: if record.get('publicname'): @@ -93,7 +109,10 @@ class ClicRdv(BaseResource): request_uri = request_uri + '&start=%s' % urlquote(date_start) if date_end: request_uri = request_uri + '&end=%s' % urlquote(date_end) - for timeslot in self.request(request_uri).json().get('availabletimeslots'): + response = self.request(request_uri) + if 'error' in response: + return response + for timeslot in response.get('availabletimeslots', []): timeslots.append(timeslot.get('start')) timeslots.sort() return timeslots @@ -135,15 +154,9 @@ class ClicRdv(BaseResource): return times def cancel(self, appointment_id, **kwargs): - try: - r = self.request('appointments/%s' % appointment_id, method='delete') - r.raise_for_status() - except requests.exceptions.HTTPError as e: - # clicrdv will return a "Bad Request" (HTTP 400) response - # when it's not possible to remove an appointment - # (for example because it's too late) - response = json.loads(str(e)) - return {'success': False, 'error': response} + response = self.request('appointments/%s' % appointment_id, method='delete') + if 'error' in response: + return response return {'success': True} @@ -185,23 +198,11 @@ class ClicRdv(BaseResource): for fieldname in (fields.keys() + extra.keys() + data.keys()): if fieldname.startswith('clicrdv_fiche_'): appointment['appointment']['fiche'][fieldname[14:]] = get_data(fieldname) or '' - try: - r = self.request('appointments', 'post', json=appointment) - r.raise_for_status() - except requests.exceptions.HTTPError as e: - try: - error = json.loads(str(e))[0].get('error') - except: - error = 'Unknown error (Passerelle)' - return { - 'success': False, - 'error': error, - } - else: - success = True - response = r.json() - appointment_id = response.get('records')[0].get('id') - return { - 'success': True, - 'appointment_id': appointment_id, - } + response = self.request('appointments', 'post', json=appointment) + if 'error' in response: + return response + appointment_id = response.get('records')[0].get('id') + return { + 'success': True, + 'appointment_id': appointment_id, + } diff --git a/tests/test_clicrdv.py b/tests/test_clicrdv.py index a80a336e..d5715dcd 100644 --- a/tests/test_clicrdv.py +++ b/tests/test_clicrdv.py @@ -164,9 +164,15 @@ def test_cancel_appointment(mocked_request, app, connector): assert mocked_request.call_count == 1 assert resp['data']['success'] -@mock.patch('passerelle.utils.Request.request', - side_effect=HTTPError('{"msg": "cancel failed"}')) +@mock.patch('passerelle.utils.Request.request') def test_failed_cancel_appointment(mocked_request, app, connector): + def raise_for_status(): + raise HTTPError("400 Client Error: Bad Request for url: xxx") + + response = mock.Mock() + response.json.return_value = [{"msg": "cancel failed"}] + response.raise_for_status = raise_for_status + mocked_request.return_value = response obj_type = ContentType.objects.get_for_model(ClicRdv) apiuser = ApiUser.objects.create(username='apiuser', keytype='API', key='apiuser') @@ -177,12 +183,18 @@ def test_failed_cancel_appointment(mocked_request, app, connector): assert mocked_request.call_count == 1 assert resp.get('err') == 0 assert resp['data'] - assert resp['data']['error'] == {'msg': 'cancel failed'} + assert 'cancel failed' in resp['data']['error'] -@mock.patch('passerelle.utils.Request.request', - side_effect=HTTPError('[{"error": "creation failed"}]')) +@mock.patch('passerelle.utils.Request.request') def test_failed_appointment_creation(mocked_request, app, connector): + def raise_for_status(): + raise HTTPError("400 Client Error: Bad Request for url: xxx") + + response = mock.Mock() + response.json.return_value = [{"msg": "creation failed"}] + response.raise_for_status = raise_for_status + mocked_request.return_value = response obj_type = ContentType.objects.get_for_model(ClicRdv) apiuser = ApiUser.objects.create(username='apiuser', keytype='API', key='apiuser') @@ -196,4 +208,4 @@ def test_failed_appointment_creation(mocked_request, app, connector): resp = app.post_json('/clicrdv/test/interventions/63258/create?apikey=apiuser', params=data).json assert resp['data'] assert not resp['data']['success'] - assert resp['data']['error'] == 'creation failed' + assert 'creation failed' in resp['data']['error'] -- 2.20.1