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

CKAN API used to validate the operationID #126

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

simran212530
Copy link

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
Copy link

codeclimate bot commented Apr 15, 2021

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

View more on Code Climate.

@andrewphilipsmith
Copy link
Member

andrewphilipsmith commented Apr 18, 2021

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:

with mock.patch('mapactionpy_controller.map_recipe.path.exists') as mock_path:

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.

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.

2 participants