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

Admin: add schema field to resource #2512

Merged
merged 12 commits into from Jul 21, 2020
Merged

Admin: add schema field to resource #2512

merged 12 commits into from Jul 21, 2020

Conversation

AntoineAugusti
Copy link
Contributor

@AntoineAugusti AntoineAugusti commented Jul 14, 2020

image

  • Link data.gouv.fr to the schema catalog of schema.data.gouv.fr
  • Add a schema field to the resource model
  • Let producers select a schema from a list on resources

I left some comments/questions at various places.

To discuss

  • Configuration for the schema catalog URL
  • The schema catalog of schema.data.gouv.fr only lists TableSchema schemas for now, should we change it? It's because we needed this endpoint for Validata, therefore it's made to include only TableSchema schemas. This can change.

js/models/schemas.js Show resolved Hide resolved
udata/core/dataset/api.py Outdated Show resolved Hide resolved
udata/core/dataset/forms.py Outdated Show resolved Hide resolved
udata/core/dataset/models.py Outdated Show resolved Hide resolved
udata/settings.py Show resolved Hide resolved
udata/tests/api/test_datasets_api.py Show resolved Hide resolved
udata/tests/api/test_datasets_api.py Show resolved Hide resolved
js/components/dataset/resource/form.vue Show resolved Hide resolved
Copy link
Contributor

@abulte abulte left a comment

Choose a reason for hiding this comment

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

nice work 🌈

udata/core/dataset/forms.py Outdated Show resolved Hide resolved
udata/core/dataset/models.py Outdated Show resolved Hide resolved
udata/core/dataset/models.py Outdated Show resolved Hide resolved
udata/tests/api/test_datasets_api.py Show resolved Hide resolved
udata/tests/dataset/test_dataset_model.py Outdated Show resolved Hide resolved
udata/tests/dataset/test_dataset_model.py Outdated Show resolved Hide resolved
udata/tests/api/test_datasets_api.py Outdated Show resolved Hide resolved
return []

r = requests.get(endpoint)
r.raise_for_status()
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 we need a smarter caching strategy and exception handling.

Something like that https://github.com/etalab/udata-gouvfr/pull/486/files#diff-1c9abf3241dab91b0e60c554f8bcd67aR72.

This would lead to removing the cache on the API and handle it here.

@abulte
Copy link
Contributor

abulte commented Jul 15, 2020

The schema catalog of schema.data.gouv.fr only lists TableSchema schemas for now, should we change it? It's because we needed this endpoint for Validata, therefore it's made to include only TableSchema schemas. This can change.

No need to change that for now. I don't think we'll do anything very interesting with other schemas for quite some time.

udata/core/dataset/forms.py Outdated Show resolved Hide resolved
udata/core/dataset/api.py Outdated Show resolved Hide resolved
udata/core/dataset/models.py Show resolved Hide resolved
@AntoineAugusti
Copy link
Contributor Author

Thanks for your guidance and your help @abulte!

I:

  • tackled your PR comments
  • reverted the commit introducing indexing - it'll be done later
  • added a double caching mechanism to fetch schemas from the schema catalog
  • cached at the model level instead of the API level
  • enforced an enumeration for the schema property at the form level

@AntoineAugusti
Copy link
Contributor Author

Added custom exceptions as you suggested @abulte. Thanks for your review.

Copy link
Contributor

@abulte abulte left a comment

Choose a reason for hiding this comment

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

One last suggestion, add a CHANGELOG entry (under "Current in progress") and 🚀

udata/core/dataset/models.py Outdated Show resolved Hide resolved
@AntoineAugusti
Copy link
Contributor Author

I had to do an extra commit this morning to fix dataset/forms.py. When starting my local server, I had a stracktrace regarding the absence of the app context in this file, because of choices=[schema['id'] for schema in ResourceSchema.objects()] which needs the app context under the hood. I guess the app context is not available yet, hence we can't access the current config at this point.

I switched to a string field with a validator. The SelectField relies on a list of options, through the choices keyword. You can't leave it empty, that's why I switched to the String class.

This pattern is used elsewhere in the very same file

def enforce_filetype_file(form, field):
'''Only allowed domains in resource.url when filetype is file'''
if form._fields.get('filetype').data != RESOURCE_FILETYPE_FILE:
return
domain = urlparse(field.data).netloc
allowed_domains = current_app.config['RESOURCES_FILE_ALLOWED_DOMAINS']
allowed_domains += [current_app.config.get('SERVER_NAME')]
if current_app.config.get('CDN_DOMAIN'):
allowed_domains.append(current_app.config['CDN_DOMAIN'])
if '*' in allowed_domains:
return
if domain and domain not in allowed_domains:
message = _('Domain "{domain}" not allowed for filetype "{filetype}"')
raise validators.ValidationError(message.format(
domain=domain, filetype=RESOURCE_FILETYPE_FILE
))

I made sure the validator was working by editing the HTML
image

@abulte abulte merged commit a9b15a0 into opendatateam:master Jul 21, 2020
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.

None yet

2 participants