mapaction / mapactionpy_controller

7 stars 6 forks source link

CKAN API used to validate the operationID #126

Open simran212530 opened 3 years ago

simran212530 commented 3 years ago

Aim:

This pull request solves the corresponding issue: CHEF_147 Jira Issue

I tried first getting familiar with the CKAN API, made the required changes for checking the validity of the operationID and then ran tests locally. If an operation ID is not valid, then an exception is raised.

codeclimate[bot] commented 3 years ago

Code Climate has analyzed commit 90253312 and detected 0 issues on this pull request.

View more on Code Climate.

andrewphilipsmith commented 3 years ago

Hi Simran, Thank you for this pull request.

Below are a few pointers for completing this PR.

We enforce pep8 on this repo using the flake8 module. See here for an example of the errors - https://travis-ci.com/github/mapaction/mapactionpy_controller/jobs/498924357. Install and running flake8 locally is a helpful way to catch these.

Looking at the errors from builds earlier in the pull request, I think there are some problems with the unit tests: https://travis-ci.com/github/mapaction/mapactionpy_controller/jobs/497938157

Ideally, the PR should include tests that demonstrate the correct behaviour of this new functionality. These should not make undue requests to a CKAN site just for the sake of testing. Because we need to maintain backward compatibility with Python 2.7, we can't use the most modern unit-testing and mock libraries. However, this is an example of a test using mocks from elsewhere in the code: https://github.com/mapaction/mapactionpy_controller/blob/f563dd13b307012f52c83690353efffde0be052e/mapactionpy_controller/tests/test_mapactionpy_controller.py#L59

Additionally, the new changes break some of the existing test (many of which use Event objects but are unrelated to the CKAN functionality). One option might be to include an optional parameter that allows the verification tests to be bypassed for test purposes but has a default value of "on". As examples both the LayerProperties and RecipeLayer have an optional verify_on_creation parameter for this purpose:

I hope that is helpful. Thank you once again for your efforts.