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

Uses new frictionless report component to display validation report #71

Merged

Conversation

aivuk
Copy link
Contributor

@aivuk aivuk commented Oct 20, 2022

Uses React Frictionless Report component to display a resource validation results.

@aivuk aivuk added the feature label Oct 20, 2022
@aivuk aivuk requested a review from amercader October 20, 2022 13:07
@aivuk aivuk self-assigned this Oct 20, 2022
@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #71 (c3b6807) into use-frictionless-framework-v5 (e774828) will decrease coverage by 0.07%.
The diff coverage is 71.42%.

@@                        Coverage Diff                        @@
##           use-frictionless-framework-v5      #71      +/-   ##
=================================================================
- Coverage                          84.38%   84.31%   -0.08%     
=================================================================
  Files                                 24       24              
  Lines                               2075     2078       +3     
=================================================================
+ Hits                                1751     1752       +1     
- Misses                               324      326       +2     
Impacted Files Coverage Δ
ckanext/validation/plugin/__init__.py 96.24% <ø> (ø)
ckanext/validation/helpers.py 88.88% <50.00%> (-1.81%) ⬇️
ckanext/validation/jobs.py 82.85% <75.00%> (-0.80%) ⬇️
ckanext/validation/tests/test_jobs.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@amercader
Copy link
Member

@aivuk This is looking good, but there are a couple of things to iron out:

  • When you click on the report page:

    • The badge shows "error" when it should show "invalid"
    • The duration of the check is not populated
    • We are showing the local path in the report, which we shouldn't. IIRC this was corrected here but it might need some adjusting to the new format
      Screenshot 2022-10-21 at 12-51-50 Validation Report - CKAN
  • When you work in sync mode (ckanext.validation.run_on_create_sync = True / ckanext.validation.run_on_update_sync = True) the files are validated in the same request where you create the resource. If there are errors, the validation report is returned. We hijack the error message in the form to not display the raw JSON report but a link instead, that opens a dialog with the report component. The component is not currently rendered in the dialog:

Screenshot 2022-10-21 at 13-04-53 Add data to the dataset - CKAN
Screenshot 2022-10-21 at 13-09-06 Add resource - Test validation 2 - CKAN

The errors.html template is a good starting point to investigate

@amercader
Copy link
Member

Sorry, just realized this was a draft, so apologies if you were already looking at these

@aivuk
Copy link
Contributor Author

aivuk commented Oct 21, 2022

Sorry, just realized this was a draft, so apologies if you were already looking at these

Thanks, I was working on the dialog, but I'm having an error using the report.js. For some reason the validation_error json is not being passed to the module-validation-report.js (it's undefined), but I checked and the json is there in the template validation/snippets/validation_report_dialog.html but it's not being passed to the JS module. I'm investigating it. I missed the error in the badge, I thought that it would need to be "error", but I got it that we need to have three states.

undefined-report-json

@aivuk
Copy link
Contributor Author

aivuk commented Oct 24, 2022

Hi @amercader, the following points that you raised are fixed:

  • The badge shows "error" when it should show "invalid"
  • The duration of the check is not populated
  • We are showing the local path in the report, which we shouldn't. IIRC this was corrected here but it might need some adjusting to the new format

I'm still debugging why when the plugin is running in sync mode, the report is not being displayed by the component. The same thing happens when the plugin is not running in sync, after you update a resource and click in the badge label before reloading the page.

@amercader
Copy link
Member

Thanks @aivuk , that sounds great. If it helps this JSON.parse() seems to be failing here but I'm not sure if that's related

README.md Outdated
@@ -72,9 +72,9 @@ Create the database tables running:

## Configuration

Once installed, add the `validation` plugin to the `ckan.plugins` configuration option in your INI file:
Once installed, add the `validation` plugin to the `ckan.plugins` configuration option in your INI file. If using ckanext-scheming, the `validation` plugins should be loaded **before** the `scheming_datasets` one:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it feasible to remove this constraint? I know that plugin ordering can be important, but perhaps there is a way to make them more independent?

Copy link
Member

Choose a reason for hiding this comment

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

@ThrawnCA I don't think so. We want ckanext-validation's errors.html to override ckanext-scheming's so we can hook the report to the error messages.

I guess that we could add "If using ckanext-scheming and using synchronous mode, the validation plugin should be loaded before the scheming_datasets one" but that would make the instructions a bit more confusing

Copy link
Contributor

@ThrawnCA ThrawnCA Oct 26, 2022

Choose a reason for hiding this comment

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

Oh, and ckanext-scheming defines the base version of that template, which doesn't extend anything, so if ckanext-scheming comes first, then processing stops and ckanext-validation's version is never reached. That makes sense.

Perhaps the reason can be documented here?

@ThrawnCA
Copy link
Contributor

Is it necessary to copy the Frictionless web assets into this repo? Are they not available in any PyPI package?

@aivuk
Copy link
Contributor Author

aivuk commented Oct 25, 2022

Is it necessary to copy the Frictionless web assets into this repo? Are they not available in any PyPI package?

The frictionless web assets are from https://components.frictionlessdata.io/ and the js package is on npm https://www.npmjs.com/package/frictionless-components. I'm using the latest build (version 1.3.2) instead of a CDN or adding a build it step on ckanext-validation that would download and build the npm package.

@aivuk aivuk marked this pull request as ready for review October 26, 2022 10:12
@aivuk
Copy link
Contributor Author

aivuk commented Oct 26, 2022

@amercader Fixed the report in sync mode, now the report is being displayed in a dialog if there are errors when creating a new resource.

@amercader
Copy link
Member

Great stuff! Can you make the dialog a bit wider? it's a bit hard to read currently. This probably helps: https://getbootstrap.com/docs/4.2/components/modal/#optional-sizes

Screenshot 2022-10-26 at 15-38-47 Add resource - Test validation 2 - CKAN

Besides this, this is good to merge once #69 is done

@aivuk
Copy link
Contributor Author

aivuk commented Oct 26, 2022

Great stuff! Can you make the dialog a bit wider? it's a bit hard to read currently. This probably helps: https://getbootstrap.com/docs/4.2/components/modal/#optional-sizes

Screenshot 2022-10-26 at 15-38-47 Add resource - Test validation 2 - CKAN

Besides this, this is good to merge once #69 is done

It's bigger now

aivuk added 2 commits October 27, 2022 09:07
…onlessdata/ckanext-validation into use-frictionless-report-component
@amercader amercader merged commit fff8dc5 into use-frictionless-framework-v5 Oct 27, 2022
@amercader amercader deleted the use-frictionless-report-component branch October 27, 2022 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants