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

Harvest config backend #1716

Merged
merged 4 commits into from Jun 5, 2018
Merged

Harvest config backend #1716

merged 4 commits into from Jun 5, 2018

Conversation

noirbizarre
Copy link
Contributor

This PR:

  • allows harvest backends to register their allowed filters
  • expose backends filters in the API
  • ensure the HarvestSource.config object is properly writable (with validation)
  • Starts testing the base backend class

Requires #1715 (included)

@@ -63,7 +63,8 @@ def create_source(name, url, backend,
description=None,
frequency=DEFAULT_HARVEST_FREQUENCY,
owner=None,
organization=None):
organization=None,
config=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move ) to a new line and add a trailing comma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -121,9 +121,19 @@ def backends_ids():
source_page_fields = api.model('HarvestSourcePage',
fields.pager(source_fields))


filter_fields = api.model('HarvestFilter', {
'label': fields.String(description='A localized human-readable label'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Localized or localizable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Localized as its the documentation for an API consumer and when he gets the field, the value is localized according to the lang parameter.

int: 'integer',
bool: 'boolean',
UUID: 'uuid',
datetime: 'date-time',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is date-time a known convention? I would have written datetime, but not important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, those are Swagger/OpenAPI/json-schema standard types

@@ -35,6 +58,10 @@ class BaseBackend(object):
display_name = None
verify_ssl = True

# Define some allowed filters on the backend
# This a Sequence[HarvestFilter]
Copy link
Contributor

Choose a reason for hiding this comment

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

double space

def get_backend(self, form):
return [
b for b in list_backends() if b.name == form.backend.data
][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe next((a for a in [1, 2]))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


def get_specs(self, backend, key):
candidates = [f for f in backend.filters if f.key == key]
return candidates[0] if len(candidates) == 1 else None
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe next((a for a in [1, 2]), None)?

if self.data:
backend = self.get_backend(form)
# Validate filters
for f in (self.data.get('filters', None) or []):
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for , None I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better self.data.get('filters', [])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly self.data.get('filters', []) doesn't cover the case where filters is None or I have to write self.data.get('filters', []) or [].
I'll go for self.data.get('filters') or []

@@ -115,6 +119,100 @@ def test_create_source_with_org_not_member(self, api):

assert403(response)

def test_create_source_with_config(self,api):
'''It should create anew source with configuration'''
Copy link
Contributor

Choose a reason for hiding this comment

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

s/anew/a new/


def get_specs(self, backend, key):
candidates = [f for f in backend.filters if f.key == key]
return candidates[0] if len(candidates) == 1 else None
return next(candidates, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need a tuple comprehension for this to work.

b for b in list_backends() if b.name == form.backend.data
][0]
])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need a tuple comprehension for this to work.

@noirbizarre noirbizarre merged commit 33b3996 into opendatateam:master Jun 5, 2018
@noirbizarre noirbizarre deleted the harvest-config-backend branch June 5, 2018 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants