Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add historical #88

Open
wants to merge 2 commits into
base: use_requests
Choose a base branch
from
Open

add historical #88

wants to merge 2 commits into from

Conversation

vladkhard
Copy link

@vladkhard vladkhard commented Jan 16, 2018

This change is Reviewable

@VDigitall
Copy link
Member

Зміни мають іти з тестами

@@ -242,8 +242,11 @@ def get_resource_item(self, id, headers=None):
return self._get_resource_item('{}/{}'.format(self.prefix_path, id),
headers=headers)

def get_resource_item_historical(self, id, headers=None):
return self._get_resource_item('{}/{}/historical'.format(self.prefix_path, id), headers=headers)
def get_resource_item_historical(self, id, revision, headers=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я б не робив revision обовязковим

Copy link
Author

@vladkhard vladkhard Jan 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Чому? Це викoристовується тільки для хісторікала, реквест без ревізії не має сенсу - щоб взяти актуальну версію документа, досить використати get_resource_item

Copy link
Member

@VDigitall VDigitall Jan 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Щоб в еджі замінити ось це

        response = client.request(
            'GET', '{}/{}/historical'.format(api_client_dict['client'].prefix_path, id)
        )
        revisions_number = int(response.headers['x-revision-n'])

@VDigitall
Copy link
Member

@vladkhard потрібно ще тести.

def side_effect(_, headers):
return item if headers["x-revision-n"] else response

self.client._get_resource_item = MagicMock(side_effect=side_effect)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.client.get_resource_item_historical(item_id, revision=0)
self.client.get_resource_item_historical(item_id, revision=revisions_limit + 1)
self.client.get_resource_item_historical(item_id, revision=None)

Copy link
Member

@VDigitall VDigitall Feb 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут краще загорнути у for і переконатись що ми дійсно отримуємо 404 всі рази.

@@ -1,12 +1,13 @@
import unittest

from openprocurement_client.tests import tests, tests_sync
from openprocurement_client.tests import tests, tests_sync, tests_api_base_client
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А де сам тест?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

упс
вже додав

@VDigitall
Copy link
Member

@vladkhard тест провалився

@coveralls
Copy link

coveralls commented Feb 14, 2018

Coverage Status

Coverage increased (+0.2%) to 85.581% when pulling df58ed2 on vladkhard:use_requests into 867fc7d on openprocurement:use_requests.

item_id, revision=revisions_limit - 1), actual_response)

for revision in (0, revisions_limit + 1, None):
try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for revision in (0, revisions_limit + 1, None):
    with self.assertRaises(InvalidResponse) as e:
        self.client.get_resource_item_historical(item_id, revision=revision)
    self.assertEqual(e.exception.status_code, 404)

@VDigitall
Copy link
Member

@VDigitall
Copy link
Member

@vladkhard, дані зміни вже не акутальні оскільки historical вміє працювати з датою, а якщо передати обидва заголовки то буде помилка.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants