search

ckan / ckanext-qa

CKAN QA Extension
MIT License
26 stars 52 forks source link

removed references to obsolete ckan.model.ResourceGroup from reports.py #8

Closed florianm closed 9 years ago

florianm commented 9 years ago

This change removes three lines from reports.py which reference the now deleted ckan.models.ResourceGroup. This gets ckanext-qa to work with CKAN 2.3a (source install of master branch on Ubuntu 14.10 LTS on Amazon EC2 t2.medium).

Two tests fail - likely more to do with my test setup.

(default)user@machine:/usr/lib/ckan/default/src# nosetests --ckan ../ckanext-qa/tests/
# throws
FAIL: test_extension.TestQAController.test_package_openness_scores
AssertionError: PATH_INFO doesn't start with /: 'qa_dataset_action'
# and
FAIL: test_extension.TestQAController.test_packages_with_broken_resource_links
AssertionError: PATH_INFO doesn't start with /: 'qa_dataset_action'

However, both paster qa update and the GUI links work.

# with CKAN's virtualenv activated
(default)user@machine:/usr/lib/ckan/default/src# paster --plugin=ckanext-qa qa update -c /path/to/my.ini

# GUI
/qa
/qa/dataset/five_stars
/qa/dataset/broken_resource_links
/api/2/util/qa/broken_resource_links_by_dataset.csv
/qa/organisation/broken_resource_links
/api/2/util/qa/organisations_with_broken_resource_links/all.csv

Before this pull request is merged, someone with a better understanding of the purpose of ResourceGroup should make sure that removing it from this extension doesn't violate assumptions or business logic I'm not aware of.

davidread commented 9 years ago

Yes this is good. The ResourceGroup is not used by anyone.

We should just need to consider backwards compatibility - ideally this fix will work with earlier versions too. Can you put in a test for the CKAN version number or existence of model.ResourceGroup?

florianm commented 9 years ago

Done, but needs testing on older CKAN versions. Testing for existence of ResourceGroup works fine on my end (CKAN 2.3 source install).

davidread commented 9 years ago

This is fine, although I'm concerned about the duplication. Does this work? https://github.com/ckan/ckanext-qa/pull/10

davidread commented 9 years ago

PS I've merged your changes to test_tasks.py

florianm commented 9 years ago

10 is a better way, I agree! Thanks!

davidread commented 9 years ago

Cheers for the help on this