From 859e809e0f93e99825fbc2bc4e4d1d67d12355f4 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Fri, 29 Jul 2022 16:09:10 +1000 Subject: [PATCH 01/15] [QOL-9161] merge upstream changes and clean up - add CKAN 2.10 compatibility - move plugin module to top level so it has a distinctive module name and simpler paths - move controller and CLI modules to better match upstream names - use ckan_cli to unify test steps --- .flake8 | 1 + .github/workflows/test.yml | 54 +++--------- README.md | 14 +-- bin/ckan_cli | 75 ++++++++++++++++ ckanext/report/blueprint.py | 37 ++++++++ ckanext/report/cli.py | 57 ++++++++++++ ckanext/report/cli/click_cli.py | 70 --------------- ckanext/report/cli/command.py | 51 ----------- .../report/{cli/paster_cli.py => command.py} | 26 ++---- .../pylons_controllers.py => controllers.py} | 2 +- ckanext/report/controllers/blueprints.py | 47 ---------- ckanext/report/helpers.py | 9 +- ckanext/report/lib.py | 17 ++-- ckanext/report/model.py | 8 +- .../report/{plugin/__init__.py => plugin.py} | 20 ++--- .../report/{cli => plugin_mixins}/__init__.py | 0 .../{plugin => plugin_mixins}/flask_plugin.py | 7 +- .../pylons_plugin.py | 7 +- ckanext/report/report_registry.py | 6 +- ckanext/report/reports.py | 4 +- .../templates/report/tagless-datasets.html | 4 +- ckanext/report/tests/test_plugin.py | 83 ++++++++++++++++++ .../{controllers/__init__.py => utils.py} | 87 +++++++++++++++++-- requirements-dev.txt => dev-requirements.txt | 0 setup.cfg | 3 - setup.py | 2 +- test.ini | 1 - 27 files changed, 407 insertions(+), 285 deletions(-) create mode 100644 bin/ckan_cli create mode 100644 ckanext/report/blueprint.py create mode 100644 ckanext/report/cli.py delete mode 100644 ckanext/report/cli/click_cli.py delete mode 100644 ckanext/report/cli/command.py rename ckanext/report/{cli/paster_cli.py => command.py} (68%) rename ckanext/report/{controllers/pylons_controllers.py => controllers.py} (87%) delete mode 100644 ckanext/report/controllers/blueprints.py rename ckanext/report/{plugin/__init__.py => plugin.py} (76%) rename ckanext/report/{cli => plugin_mixins}/__init__.py (100%) rename ckanext/report/{plugin => plugin_mixins}/flask_plugin.py (57%) rename ckanext/report/{plugin => plugin_mixins}/pylons_plugin.py (75%) create mode 100644 ckanext/report/tests/test_plugin.py rename ckanext/report/{controllers/__init__.py => utils.py} (64%) rename requirements-dev.txt => dev-requirements.txt (100%) diff --git a/.flake8 b/.flake8 index d6b0f6b..73186fc 100644 --- a/.flake8 +++ b/.flake8 @@ -10,6 +10,7 @@ format = pylint # Show the source of errors. show_source = True +statistics = True max-complexity = 10 max-line-length = 127 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 43efa8a..5f6d079 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -5,30 +5,20 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - uses: actions/setup-python@v2 with: python-version: '3.6' - - name: Cache pip - uses: actions/cache@v2 - with: - # This path is specific to Ubuntu - path: ~/.cache/pip - # Look to see if there is a cache hit for the corresponding requirements file - key: ${{ runner.os }}-pip-flake8-${{ hashFiles('requirements.txt') }} - restore-keys: | - ${{ runner.os }}-pip-flake8- - ${{ runner.os }}- - name: Install requirements run: pip install flake8 pycodestyle - name: Check syntax - run: flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics --exclude ckan + run: flake8 test: needs: lint strategy: matrix: - ckan-version: [2.9, 2.9-py2, 2.8, 2.7] + ckan-version: ["2.10", 2.9, 2.9-py2, 2.8, 2.7] fail-fast: false name: CKAN ${{ matrix.ckan-version }} @@ -37,7 +27,7 @@ jobs: image: openknowledge/ckan-dev:${{ matrix.ckan-version }} services: solr: - image: ckan/ckan-solr-dev:${{ matrix.ckan-version }} + image: ckan/ckan-solr:${{ matrix.ckan-version }} postgres: image: ckan/ckan-postgres-dev:${{ matrix.ckan-version }} env: @@ -46,7 +36,7 @@ jobs: POSTGRES_DB: postgres options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5 redis: - image: redis:3 + image: redis:3 env: CKAN_SQLALCHEMY_URL: postgresql://ckan_default:pass@postgres/ckan_test CKAN_DATASTORE_WRITE_URL: postgresql://datastore_write:pass@postgres/datastore_test @@ -55,38 +45,20 @@ jobs: CKAN_REDIS_URL: redis://redis:6379/1 steps: - - uses: actions/checkout@v2 - - - name: Cache pip - uses: actions/cache@v2 - with: - # This path is specific to Ubuntu - path: ~/.cache/pip - # Look to see if there is a cache hit for the corresponding requirements file - key: ${{ runner.os }}-pip-${{ hashFiles('*requirements.txt') }} - restore-keys: | - ${{ runner.os }}-pip- - ${{ runner.os }}- - + - uses: actions/checkout@v3 - name: Install requirements run: | - pip install -r requirements-dev.txt + pip install -r dev-requirements.txt pip install -e . # Replace default path to CKAN core config file with the one on the container sed -i -e 's/use = config:.*/use = config:\/srv\/app\/src\/ckan\/test-core.ini/' test.ini - - name: Setup extension (CKAN >= 2.9) - if: ${{ matrix.ckan-version != '2.7' && matrix.ckan-version != '2.8' }} - run: | - ckan -c test.ini db init - ckan -c test.ini report initdb - ckan -c test.ini report generate tagless-datasets - - name: Setup extension (CKAN < 2.9) - if: ${{ matrix.ckan-version == '2.7' || matrix.ckan-version == '2.8' }} run: | - paster --plugin=ckan db init -c test.ini - paster --plugin=ckanext-report report initdb -c test.ini - paster --plugin=ckanext-report report generate tagless-datasets -c test.ini - + CKAN_CLI=bin/ckan_cli + chmod u+x $CKAN_CLI + export CKAN_INI=test.ini + $CKAN_CLI db init + PASTER_PLUGIN=ckanext-report $CKAN_CLI report initdb + PASTER_PLUGIN=ckanext-report $CKAN_CLI report generate tagless-datasets - name: Run tests run: pytest --ckan-ini=test.ini --cov=ckanext.report --disable-warnings ckanext/report/tests diff --git a/README.md b/README.md index cd935cb..97d63e3 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,5 @@ [![Tests](https://github.com/qld-gov-au/ckanext-report/actions/workflows/test.yml/badge.svg)](https://github.com/qld-gov-au/ckanext-report/actions/workflows/test.yml) -ckanext-report +# ckanext-report ==================== ckanext-report is a CKAN extension that provides a reporting infrastructure. Here are the features offered: @@ -28,7 +28,9 @@ TODO: | 2.6 and earlier | yes | | 2.7 | yes | | 2.8 | yes | -| 2.9 | yes | +| 2.9-py2 | yes | +| 2.9 (py3) | yes | +| 2.10 (py3) | yes | Status: was in production at data.gov.uk around 2014-2016, but since that uses its own CSS rather than core CKAN's, for others to use it CSS needs adding. For an example, see this branch: see https://github.com/GSA/ckanext-report/tree/geoversion @@ -43,7 +45,8 @@ Install ckanext-report into your CKAN virtual environment in the usual way: Initialize the database tables needed by ckanext-report: - (pyenv) $ paster --plugin=ckanext-report report initdb --config=mysite.ini + CKAN < 2.9 (pyenv) $ paster --plugin=ckanext-report report initdb --config=mysite.ini + CKAN >= 2.9 (pyenv) $ ckan -c mysite.ini report initdb Enable the plugin. In your config (e.g. development.ini or production.ini) add ``report`` to your ckan.plugins. e.g.: @@ -230,10 +233,7 @@ class TaglessReportPlugin(p.SingletonPlugin): The last line refers to `tag_report_info` which is a dictionary with properties of the report. This is stored in `reports.py` together with the report code (see above). The info dict looks like this: ```python -try: - from collections import OrderedDict # from python 2.7 -except ImportError: - from sqlalchemy.util import OrderedDict +from collections import OrderedDict tagless_report_info = { 'name': 'tagless-datasets', 'description': 'Datasets which have no tags.', diff --git a/bin/ckan_cli b/bin/ckan_cli new file mode 100644 index 0000000..7757dc8 --- /dev/null +++ b/bin/ckan_cli @@ -0,0 +1,75 @@ +#!/bin/sh + +# Call either 'ckan' (from CKAN >= 2.9) or 'paster' (from CKAN <= 2.8) +# with appropriate syntax, depending on what is present on the system. +# This is intended to smooth the upgrade process from 2.8 to 2.9. +# Eg: +# ckan_cli jobs list +# could become either: +# paster --plugin=ckan jobs list -c /etc/ckan/default/production.ini +# or: +# ckan -c /etc/ckan/default/production.ini jobs list + +# This script is aware of the VIRTUAL_ENV environment variable, and will +# attempt to respect it with similar behaviour to commands like 'pip'. +# Eg placing this script in a virtualenv 'bin' directory will cause it +# to call the 'ckan' or 'paster' command in that directory, while +# placing this script elsewhere will cause it to rely on the VIRTUAL_ENV +# variable, or if that is not set, the system PATH. + +# Since the positioning of the CKAN configuration file is central to the +# differences between 'paster' and 'ckan', this script needs to be aware +# of the config file location. It will use the CKAN_INI environment +# variable if it exists, or default to /etc/ckan/default/production.ini. + +# If 'paster' is being used, the default plugin is 'ckan'. A different +# plugin can be specified by setting the PASTER_PLUGIN environment +# variable. This variable is irrelevant if using the 'ckan' command. + +CKAN_INI="${CKAN_INI:-/etc/ckan/default/production.ini}" +PASTER_PLUGIN="${PASTER_PLUGIN:-ckan}" +# First, look for a command alongside this file +ENV_DIR=$(dirname "$0") +if [ -f "$ENV_DIR/ckan" ]; then + COMMAND=ckan +elif [ -f "$ENV_DIR/paster" ]; then + COMMAND=paster +elif [ "$VIRTUAL_ENV" != "" ]; then + # If command not found alongside this file, check the virtualenv + ENV_DIR="$VIRTUAL_ENV/bin" + if [ -f "$ENV_DIR/ckan" ]; then + COMMAND=ckan + elif [ -f "$ENV_DIR/paster" ]; then + COMMAND=paster + fi +else + # if no virtualenv is active, try the system path + if (which ckan > /dev/null 2>&1); then + ENV_DIR=$(dirname $(which ckan)) + COMMAND=ckan + elif (which paster > /dev/null 2>&1); then + ENV_DIR=$(dirname $(which paster)) + COMMAND=paster + else + echo "Unable to locate 'ckan' or 'paster' command" >&2 + exit 1 + fi +fi + +if [ "$COMMAND" = "ckan" ]; then + # adjust args to match ckan expectations + COMMAND=$(echo "$1" | sed -e 's/create-test-data/seed/') + echo "Using 'ckan' command from $ENV_DIR with config ${CKAN_INI} to run $COMMAND..." >&2 + shift + exec $ENV_DIR/ckan -c ${CKAN_INI} $COMMAND "$@" $CLICK_ARGS +elif [ "$COMMAND" = "paster" ]; then + # adjust args to match paster expectations + COMMAND=$1 + echo "Using 'paster' command from $ENV_DIR with config ${CKAN_INI} to run $COMMAND..." >&2 + shift + if [ "$1" = "show" ]; then shift; fi + exec $ENV_DIR/paster --plugin=$PASTER_PLUGIN $COMMAND "$@" -c ${CKAN_INI} +else + echo "Unable to locate 'ckan' or 'paster' command in $ENV_DIR" >&2 + exit 1 +fi diff --git a/ckanext/report/blueprint.py b/ckanext/report/blueprint.py new file mode 100644 index 0000000..35fd82c --- /dev/null +++ b/ckanext/report/blueprint.py @@ -0,0 +1,37 @@ +# encoding: utf-8 + +import six + +from flask import Blueprint, make_response + +from ckan.plugins import toolkit + +from .utils import report_index as index, report_view + + +report = Blueprint(u'report', __name__) + + +def redirect_to_index(): + return toolkit.redirect_to('/report') + + +def view(report_name, organization=None, refresh=False): + body, headers = report_view(report_name, organization, refresh) + if headers: + response = make_response(body) + for key, value in six.iteritems(headers): + response.headers[key] = value + return response + else: + return body + + +report.add_url_rule(u'/report', view_func=index) +report.add_url_rule(u'/reports', 'reports', view_func=redirect_to_index) +report.add_url_rule(u'/report/', view_func=view, methods=('GET', 'POST')) +report.add_url_rule(u'/report//', 'org', view_func=view, methods=('GET', 'POST',)) + + +def get_blueprints(): + return [report] diff --git a/ckanext/report/cli.py b/ckanext/report/cli.py new file mode 100644 index 0000000..f7436ee --- /dev/null +++ b/ckanext/report/cli.py @@ -0,0 +1,57 @@ +# encoding: utf-8 + +import click + +from . import utils + + +def get_commands(): + return [report] + + +@click.group() +def report(): + """Generates reports""" + pass + + +@report.command() +def initdb(): + """Creates necessary db tables""" + utils.initdb() + click.secho(u'Report table is setup', fg=u"green") + + +@report.command() +@click.argument(u'report_list', required=False) +def generate(report_list): + """ + Generate and cache reports - all of them unless you specify + a comma separated list of them. + """ + if report_list: + report_list = [s.strip() for s in report_list.split(',')] + timings = utils.generate(report_list) + + click.secho(u'Report generation complete %s' % timings, fg=u"green") + + +@report.command() +def list(): + """ Lists the reports + """ + utils.list() + + +@report.command() +@click.argument(u'report_name') +@click.argument(u'report_options', nargs=-1) +def generate_for_options(report_name, report_options): + """ + Generate and cache a report for one combination of option values. + You can leave it with the defaults or specify options + as more parameters: key1=value key2=value + """ + message = utils.generate_for_options(report_name, report_options) + if message: + click.secho(message, fg=u"red") diff --git a/ckanext/report/cli/click_cli.py b/ckanext/report/cli/click_cli.py deleted file mode 100644 index 314b42a..0000000 --- a/ckanext/report/cli/click_cli.py +++ /dev/null @@ -1,70 +0,0 @@ -# encoding: utf-8 - -import click - -from ckanext.report.cli.command import Reporting - -# Click commands for CKAN 2.9 and above - - -@click.group() -def report(): - """ XLoader commands - """ - pass - - -@report.command() -def list(): - """ Lists the reports - """ - cmd = Reporting() - cmd.list() - - -@report.command() -def initdb(): - """ Initialize the database tables for this extension - """ - cmd = Reporting() - cmd.initdb() - - -@report.command() -@click.argument(u'report_names') -def generate(report_names): - """ - Generate and cache reports - all of them unless you specify - a comma separated list of them. - """ - cmd = Reporting() - report_list = [s.strip() for s in report_names.split(',')] - cmd.generate(report_list) - - -@report.command() -@click.argument(u'report_name') -@click.argument(u'options', nargs=-1) -def generate_for_options(report_name, options): - """ - Generate and cache a report for one combination of option values. - You can leave it with the defaults or specify options - as more parameters: key1=value key2=value - """ - cmd = Reporting() - report_options = {} - for option_arg in options: - if '=' not in option_arg: - raise click.BadParameter( - 'Option needs an "=" sign in it', - options) - equal_pos = option_arg.find('=') - key, value = option_arg[:equal_pos], option_arg[equal_pos + 1:] - if value == '': - value = None # this is what the web i/f does with params - report_options[key] = value - cmd.generate_for_options(report_name, report_options) - - -def get_commands(): - return [report] diff --git a/ckanext/report/cli/command.py b/ckanext/report/cli/command.py deleted file mode 100644 index cbf5439..0000000 --- a/ckanext/report/cli/command.py +++ /dev/null @@ -1,51 +0,0 @@ -# encoding: utf-8 - -import logging -import time - - -class Reporting(): - - def __init__(self): - self.log = logging.getLogger("ckanext.report.cli") - - def initdb(self): - from ckanext.report import model - model.init_tables() - self.log.info('Report table is setup') - - def list(self): - from ckanext.report.report_registry import ReportRegistry - registry = ReportRegistry.instance() - for plugin, report_name, report_title in registry.get_names(): - report = registry.get_report(report_name) - date = report.get_cached_date() - print('%s: %s %s' % (plugin, report_name, - date.strftime('%d/%m/%Y %H:%M') if date else '(not cached)')) - - def generate(self, report_list=None): - from ckanext.report.report_registry import ReportRegistry - timings = {} - - self.log.info("Running reports => %s", report_list) - registry = ReportRegistry.instance() - if report_list: - for report_name in report_list: - s = time.time() - registry.get_report(report_name).refresh_cache_for_all_options() - timings[report_name] = time.time() - s - else: - s = time.time() - registry.refresh_cache_for_all_reports() - timings["All Reports"] = time.time() - s - - self.log.info("Report generation complete %s", timings) - - def generate_for_options(self, report_name, options): - from ckanext.report.report_registry import ReportRegistry - self.log.info("Running report => %s", report_name) - registry = ReportRegistry.instance() - report = registry.get_report(report_name) - all_options = report.add_defaults_to_options(options, - report.option_defaults) - report.refresh_cache(all_options) diff --git a/ckanext/report/cli/paster_cli.py b/ckanext/report/command.py similarity index 68% rename from ckanext/report/cli/paster_cli.py rename to ckanext/report/command.py index f656408..8714799 100644 --- a/ckanext/report/cli/paster_cli.py +++ b/ckanext/report/command.py @@ -2,7 +2,7 @@ import ckan.plugins as p -from ckanext.report.cli.command import Reporting +from . import utils class ReportCommand(p.toolkit.CkanCommand): @@ -56,28 +56,20 @@ def command(self): self.log = logging.getLogger("ckan.lib.cli") cmd = self.args[0] - reporter = Reporting() if cmd == 'initdb': - reporter.initdb() + utils.initdb() elif cmd == 'list': - reporter.list() + utils.list() elif cmd == 'generate': report_list = None if len(self.args) == 2: report_list = [s.strip() for s in self.args[1].split(',')] - reporter.generate(report_list) + self.log.info("Running reports => %s", report_list) + timings = utils.generate(report_list) + self.log.info("Report generation complete %s", timings) elif cmd == 'generate-for-options': - report_name = self.args[1] - report_options = {} - for option_arg in self.args[2:]: - if '=' not in option_arg: - self.parser.error('Option needs an "=" sign in it: "%s"' - % option_arg) - equal_pos = option_arg.find('=') - key, value = option_arg[:equal_pos], option_arg[equal_pos + 1:] - if value == '': - value = None # this is what the web i/f does with params - report_options[key] = value - reporter.generate_for_options(report_name, report_options) + message = utils.generate_for_options(self.args[1], self.self.args[2:]) + if message: + self.parser.error(message) else: self.parser.error('Command not recognized: %r' % cmd) diff --git a/ckanext/report/controllers/pylons_controllers.py b/ckanext/report/controllers.py similarity index 87% rename from ckanext/report/controllers/pylons_controllers.py rename to ckanext/report/controllers.py index 82517a6..60f61d8 100644 --- a/ckanext/report/controllers/pylons_controllers.py +++ b/ckanext/report/controllers.py @@ -3,7 +3,7 @@ import six import ckan.plugins.toolkit as t -from ckanext.report.controllers import report_index, report_view +from .utils import report_index, report_view class ReportController(t.BaseController): diff --git a/ckanext/report/controllers/blueprints.py b/ckanext/report/controllers/blueprints.py deleted file mode 100644 index ba109a7..0000000 --- a/ckanext/report/controllers/blueprints.py +++ /dev/null @@ -1,47 +0,0 @@ -# encoding: utf-8 - -import six - -from flask import Blueprint, Response - -from ckan.plugins import toolkit -from . import report_index, report_view - - -reporting = Blueprint( - u'report', - __name__ -) - - -def redirect_to_index(): - return toolkit.redirect_to('/report') - - -def view(report_name, organization=None, refresh=False): - body, headers = report_view(report_name, organization, refresh) - if headers: - response = Response(body) - for key, value in six.iteritems(headers): - response.headers[key] = value - return response - else: - return body - - -reporting.add_url_rule( - u'/report', 'index', view_func=report_index -) -reporting.add_url_rule( - u'/reports', 'reports', view_func=redirect_to_index -) -reporting.add_url_rule( - u'/report/', 'view', view_func=view, methods=('GET', 'POST',) -) -reporting.add_url_rule( - u'/report//', 'org', view_func=view, methods=('GET', 'POST',) -) - - -def get_blueprints(): - return [reporting] diff --git a/ckanext/report/helpers.py b/ckanext/report/helpers.py index dca49f8..34339e9 100644 --- a/ckanext/report/helpers.py +++ b/ckanext/report/helpers.py @@ -18,7 +18,6 @@ def relative_url_for(**kwargs): 'protocol', 'qualified')) user_specified_params = [(k, v) for k, v in list(tk.request.params.items()) if k not in disallowed_params] - if tk.check_ckan_version(min_version="2.9.0"): from flask import request args = dict(list(request.args.items()) @@ -88,3 +87,11 @@ def explicit_default_options(report_name): if options[key] is True: explicit_defaults[key] = 1 return explicit_defaults + + +def is_ckan_29(): + """ + Returns True if using CKAN 2.9+, with Flask and Webassets. + Returns False if those are not present. + """ + return tk.check_ckan_version(min_version='2.9.0') diff --git a/ckanext/report/lib.py b/ckanext/report/lib.py index 1c8f818..987d696 100644 --- a/ckanext/report/lib.py +++ b/ckanext/report/lib.py @@ -1,15 +1,16 @@ ''' These functions are for use by other extensions for their reports. ''' -import datetime +from datetime import datetime import six -from six.moves import zip +from six.moves import cStringIO as StringIO, zip try: from collections import OrderedDict # from python 2.7 except ImportError: from sqlalchemy.util import OrderedDict import ckan.plugins as p +from ckan.plugins.toolkit import config def all_organizations(include_none=False): @@ -59,10 +60,6 @@ def filter_by_organizations(query, organization, include_sub_organizations): def dataset_notes(pkg): '''Returns a string with notes about the given package. It is configurable.''' - if p.toolkit.check_ckan_version(min_version="2.6.0"): - from ckan.plugins.toolkit import config - else: - from pylons import config expression = config.get('ckanext-report.notes.dataset') if not expression: return '' @@ -72,13 +69,13 @@ def dataset_notes(pkg): def percent(numerator, denominator): if denominator == 0: return 100 if numerator else 0 - return int((numerator * 100.0) // denominator) + return int((numerator * 100.0) / denominator) def make_csv_from_dicts(rows): import csv - csvout = six.StringIO() + csvout = StringIO() csvwriter = csv.writer( csvout, dialect='excel', @@ -99,9 +96,9 @@ def make_csv_from_dicts(rows): items = [] for header in headers_ordered: item = row.get(header, 'no record') - if isinstance(item, datetime.datetime): + if isinstance(item, datetime): item = item.strftime('%Y-%m-%d %H:%M') - elif isinstance(item, (int, int, float, list, tuple)): + elif isinstance(item, (int, float, list, tuple)): item = six.text_type(item) elif item is None: item = '' diff --git a/ckanext/report/model.py b/ckanext/report/model.py index 986b7a1..d8c23df 100644 --- a/ckanext/report/model.py +++ b/ckanext/report/model.py @@ -1,4 +1,4 @@ -from __future__ import absolute_import +# encoding: utf-8 import datetime import logging @@ -79,7 +79,6 @@ def get(cls, object_id, key, convert_json=False, max_age=None): .filter(cls.object_id == object_id)\ .first() if not item: - # log.debug('Does not exist in cache: %s/%s', object_id, key) return (None, None) if max_age: @@ -91,7 +90,9 @@ def get(cls, object_id, key, convert_json=False, max_age=None): value = item.value if convert_json: - # Preserve the order of the columns in the data + # Use OrderedDict instead of dict, so that the order of the columns + # in the data is preserved from the data when it was written (assuming + # it was written as an OrderedDict in the report's code). try: # Python 2.7's json library has object_pairs_hook import json @@ -100,7 +101,6 @@ def get(cls, object_id, key, convert_json=False, max_age=None): # Python 2.4-2.6 import simplejson as json value = json.loads(value, object_pairs_hook=OrderedDict) - # log.debug('Cache load: %s/%s "%s"...', object_id, key, repr(value)[:40]) return value, item.created @classmethod diff --git a/ckanext/report/plugin/__init__.py b/ckanext/report/plugin.py similarity index 76% rename from ckanext/report/plugin/__init__.py rename to ckanext/report/plugin.py index ddbeb29..fd40b48 100644 --- a/ckanext/report/plugin/__init__.py +++ b/ckanext/report/plugin.py @@ -2,18 +2,16 @@ import ckan.plugins as p from ckan.plugins import toolkit -from ckanext.report.interfaces import IReport -import ckanext.report.logic.action.get as action_get -import ckanext.report.logic.action.update as action_update -import ckanext.report.logic.auth.get as auth_get -import ckanext.report.logic.auth.update as auth_update +from . import helpers as h +from .interfaces import IReport +from .logic.action import get as action_get, update as action_update +from .logic.auth import get as auth_get, update as auth_update - -if toolkit.check_ckan_version("2.9"): - from ckanext.report.plugin.flask_plugin import MixinPlugin +if h.is_ckan_29(): + from .plugin_mixins.flask_plugin import MixinPlugin else: - from ckanext.report.plugin.pylons_plugin import MixinPlugin + from .plugin_mixins.pylons_plugin import MixinPlugin class ReportPlugin(MixinPlugin, p.SingletonPlugin): @@ -25,18 +23,18 @@ class ReportPlugin(MixinPlugin, p.SingletonPlugin): # IConfigurer def update_config(self, config): - toolkit.add_template_directory(config, '../templates') + toolkit.add_template_directory(config, 'templates') # ITemplateHelpers def get_helpers(self): - from ckanext.report import helpers as h return { 'report__relative_url_for': h.relative_url_for, 'report__chunks': h.chunks, 'report__organization_list': h.organization_list, 'report__render_datetime': h.render_datetime, 'report__explicit_default_options': h.explicit_default_options, + 'is_ckan_29': h.is_ckan_29(), } # IActions diff --git a/ckanext/report/cli/__init__.py b/ckanext/report/plugin_mixins/__init__.py similarity index 100% rename from ckanext/report/cli/__init__.py rename to ckanext/report/plugin_mixins/__init__.py diff --git a/ckanext/report/plugin/flask_plugin.py b/ckanext/report/plugin_mixins/flask_plugin.py similarity index 57% rename from ckanext/report/plugin/flask_plugin.py rename to ckanext/report/plugin_mixins/flask_plugin.py index 0bfca3f..9702341 100644 --- a/ckanext/report/plugin/flask_plugin.py +++ b/ckanext/report/plugin_mixins/flask_plugin.py @@ -1,8 +1,7 @@ # encoding: utf-8 import ckan.plugins as p -from ckanext.report.controllers import blueprints -from ckanext.report.cli import click_cli +from ckanext.report import blueprint, cli class MixinPlugin(p.SingletonPlugin): @@ -12,9 +11,9 @@ class MixinPlugin(p.SingletonPlugin): # IBlueprint def get_blueprint(self): - return blueprints.get_blueprints() + return blueprint.get_blueprints() # IClick def get_commands(self): - return click_cli.get_commands() + return cli.get_commands() diff --git a/ckanext/report/plugin/pylons_plugin.py b/ckanext/report/plugin_mixins/pylons_plugin.py similarity index 75% rename from ckanext/report/plugin/pylons_plugin.py rename to ckanext/report/plugin_mixins/pylons_plugin.py index 44136ca..37f33a0 100644 --- a/ckanext/report/plugin/pylons_plugin.py +++ b/ckanext/report/plugin_mixins/pylons_plugin.py @@ -9,8 +9,9 @@ class MixinPlugin(p.SingletonPlugin): # IRoutes def before_map(self, map): - report_ctlr = 'ckanext.report.controllers.pylons_controllers:ReportController' - map.connect('report.index', '/report', controller=report_ctlr, action='index') + report_ctlr = 'ckanext.report.controllers:ReportController' + map.connect('report.index', '/report', controller=report_ctlr, + action='index') map.connect('reports', '/report', controller=report_ctlr, action='index') map.redirect('/reports', '/report') @@ -18,6 +19,8 @@ def before_map(self, map): action='view') map.connect('report', '/report/:report_name', controller=report_ctlr, action='view') + map.connect('report-org', '/report/:report_name/:organization', + controller=report_ctlr, action='view') map.connect('report.org', '/report/:report_name/:organization', controller=report_ctlr, action='view') return map diff --git a/ckanext/report/report_registry.py b/ckanext/report/report_registry.py index ed1d24b..048b7b2 100644 --- a/ckanext/report/report_registry.py +++ b/ckanext/report/report_registry.py @@ -7,12 +7,13 @@ from ckan import model from ckan.plugins.toolkit import asbool -from ckanext.report.interfaces import IReport try: from collections import OrderedDict # from python 2.7 except ImportError: from sqlalchemy.util import OrderedDict +from ckanext.report.interfaces import IReport + log = logging.getLogger(__name__) REPORT_KEYS_REQUIRED = set(('name', 'generate', 'template', 'option_defaults', @@ -29,8 +30,7 @@ def __init__(self, report_info_dict, plugin): missing_required_keys = REPORT_KEYS_REQUIRED - set(report_info_dict.keys()) assert not missing_required_keys, 'Report info dict missing keys %r: '\ '%r' % (missing_required_keys, report_info_dict) - unknown_keys = set(report_info_dict.keys()) - REPORT_KEYS_REQUIRED - \ - REPORT_KEYS_OPTIONAL + unknown_keys = set(report_info_dict.keys()) - REPORT_KEYS_REQUIRED - REPORT_KEYS_OPTIONAL assert not unknown_keys, 'Report info dict has unrecognized keys %r: '\ '%r' % (unknown_keys, report_info_dict) if not report_info_dict['option_defaults']: diff --git a/ckanext/report/reports.py b/ckanext/report/reports.py index cf11b3c..49dc7c0 100644 --- a/ckanext/report/reports.py +++ b/ckanext/report/reports.py @@ -3,12 +3,14 @@ ''' from ckan import model -from ckanext.report import lib + try: from collections import OrderedDict # from python 2.7 except ImportError: from sqlalchemy.util import OrderedDict +from ckanext.report import lib + def tagless_report(organization, include_sub_organizations=False): ''' diff --git a/ckanext/report/templates/report/tagless-datasets.html b/ckanext/report/templates/report/tagless-datasets.html index 0a75c8a..4e0d4b6 100644 --- a/ckanext/report/templates/report/tagless-datasets.html +++ b/ckanext/report/templates/report/tagless-datasets.html @@ -4,6 +4,8 @@ table - main data, as a list of rows, each row is a dict data - other data values, as a dict #} + +{% set dataset_read_route = 'dataset.read' if h.is_ckan_29() else 'dataset_read' %}
  • {% trans %}Datasets without tags{% endtrans %}: {{ table|length }} / {{ data['num_packages'] }} ({{ data['packages_without_tags_percent'] }})
  • {% trans %}Average tags per package{% endtrans %}: {{ data['average_tags_per_package'] }} tags
    • @@ -22,7 +24,7 @@ {% for row in table %} - + {{ row.title }} diff --git a/ckanext/report/tests/test_plugin.py b/ckanext/report/tests/test_plugin.py new file mode 100644 index 0000000..307a768 --- /dev/null +++ b/ckanext/report/tests/test_plugin.py @@ -0,0 +1,83 @@ +import pytest +import six +from ckan.plugins import toolkit as tk +from ckan.tests import factories +import ckanext.report.model as report_model + + +def _assert_in_body(string, response): + if six.PY2: + assert string in response.body.decode('utf8') + else: + assert string in response.body + + +def _assert_status(res, code_int): + if hasattr(res, 'status_code'): + assert res.status_code == code_int + elif hasattr(res, 'status_int'): + assert res.status_int == code_int + else: + raise NotImplementedError('No status for response') + + +@pytest.fixture +def report_setup(): + report_model.init_tables() + + +@pytest.mark.ckan_config(u'ckan.plugins', u'report tagless_report') +@pytest.mark.usefixtures(u'clean_db', u'with_plugins', u'report_setup') +class TestReportPlugin(object): + + def test_report_routes(self, app): + u"""Test report routes""" + res = app.get(u'/report') + + _assert_in_body(u"Reports", res) + + def test_tagless_report_listed(self, app): + u"""Test tagless report is listed on report page""" + res = app.get(u'/report') + + _assert_in_body(u'Tagless datasets', res) + _assert_in_body(u'href="/report/tagless-datasets"', res) + + def test_tagless_report(self, app): + u"""Test tagless report generation""" + dataset = factories.Dataset() + + res = app.get(u'/report/tagless-datasets') + + _assert_in_body(u"Datasets which have no tags.", res) + _assert_in_body('href="/dataset/' + dataset['name'] + '"', res) + + def test_tagless_report_csv(self, app): + u"""Test tagless report generation""" + dataset1 = factories.Dataset() # noqa F841 + dataset2 = factories.Dataset() # noqa F841 + + res = app.get(u'/report/tagless-datasets?format=csv') + _assert_status(res, 200) + + def test_tagless_report_json(self, app): + u"""Test tagless report generation""" + dataset1 = factories.Dataset() # noqa F841 + dataset2 = factories.Dataset() # noqa F841 + res = app.get(u'/report/tagless-datasets?format=json') + _assert_status(res, 200) + + def test_tagless_report_refresh_ok(self, app): + u"""Test tagless refresh report""" + user = factories.Sysadmin() + dataset = factories.Dataset() # noqa F841 + env = {'REMOTE_USER': user['name'].encode('ascii')} + + res = app.post('/report/tagless-datasets', extra_environ=env) + + # for CKAN >= 2.9, the response is not a redirect + if tk.check_ckan_version(min_version="2.9.0"): + _assert_status(res, 200) + else: + _assert_status(res, 302) + assert res.headers.get('Location') == 'http://localhost:5000/report/tagless-datasets' diff --git a/ckanext/report/controllers/__init__.py b/ckanext/report/utils.py similarity index 64% rename from ckanext/report/controllers/__init__.py rename to ckanext/report/utils.py index 820d496..9d3e39b 100644 --- a/ckanext/report/controllers/__init__.py +++ b/ckanext/report/utils.py @@ -1,19 +1,30 @@ # encoding: utf-8 +import logging import six +import time -from ckan.common import request from ckan.lib.helpers import json -from ckan.lib.render import TemplateNotFound +try: + from jinja2.exceptions import TemplateNotFound +except ImportError: + # CKAN 2.8 + from ckan.lib.render import TemplateNotFound import ckan.plugins.toolkit as t -import ckanext.report.helpers as helpers -from ckanext.report.lib import make_csv_from_dicts, ensure_data_is_dicts, anonymise_user_names -from ckanext.report.report_registry import Report +from ckan.plugins.toolkit import request -log = __import__('logging').getLogger(__name__) +from . import helpers +from .lib import make_csv_from_dicts, ensure_data_is_dicts, anonymise_user_names +from .report_registry import Report, ReportRegistry + +log = logging.getLogger(__name__) c = t.c +############################################################################### +# Controller # +############################################################################### + def _get_routing_rule(): if hasattr(request, 'url_rule'): @@ -51,13 +62,13 @@ def report_view(report_name, organization=None, refresh=False): and report['option_defaults']['organization']: org = report['option_defaults']['organization'] return t.redirect_to(helpers.relative_url_for(organization=org)) - if 'organization' in t.request.params: + if 'organization' in request.params: # organization should only be in the url - let the param overwrite # the url. return t.redirect_to(helpers.relative_url_for()) # options - options = Report.add_defaults_to_options(t.request.params, report['option_defaults']) + options = Report.add_defaults_to_options(request.params, report['option_defaults']) option_display_params = {} if 'format' in options: format = options.pop('format') @@ -86,7 +97,7 @@ def report_view(report_name, organization=None, refresh=False): # Alternative way to refresh the cache - not in the UI, but is # handy for testing try: - refresh = t.asbool(t.request.params.get('refresh')) + refresh = t.asbool(request.params.get('refresh')) if 'refresh' in options: options.pop('refresh') except ValueError: @@ -147,3 +158,61 @@ def report_view(report_name, organization=None, refresh=False): 'options_html': options_html, 'report_template': report['template'], 'are_some_results': are_some_results}), {} + + +############################################################################### +# CLI # +############################################################################### + + +def initdb(): + from ckanext.report import model + model.init_tables() + log.info('Report table is setup') + + +def generate(report_list): + timings = {} + + registry = ReportRegistry.instance() + if report_list: + log.info("Running reports => %s", report_list) + for report_name in report_list: + s = time.time() + registry.get_report(report_name).refresh_cache_for_all_options() + timings[report_name] = time.time() - s + else: + s = time.time() + registry.refresh_cache_for_all_reports() + timings["All Reports"] = time.time() - s + + return timings + + +def list(): + registry = ReportRegistry.instance() + for plugin, report_name, report_title in registry.get_names(): + report = registry.get_report(report_name) + date = report.get_cached_date() + print('%s: %s %s' % (plugin, report_name, + date.strftime('%d/%m/%Y %H:%M') if date else '(not cached)')) + + +def generate_for_options(report_name, options): + report_options = {} + + for option_arg in options: + if '=' not in option_arg: + return 'Option needs an "=" sign in it: "%s"' % option_arg + equal_pos = option_arg.find('=') + key, value = option_arg[:equal_pos], option_arg[equal_pos + 1:] + if value == '': + value = None # this is what the web i/f does with params + report_options[key] = value + + log.info("Running report => %s", report_name) + registry = ReportRegistry.instance() + report = registry.get_report(report_name) + all_options = report.add_defaults_to_options(report_options, + report.option_defaults) + report.refresh_cache(all_options) diff --git a/requirements-dev.txt b/dev-requirements.txt similarity index 100% rename from requirements-dev.txt rename to dev-requirements.txt diff --git a/setup.cfg b/setup.cfg index 63fb8cb..45a4e75 100644 --- a/setup.cfg +++ b/setup.cfg @@ -20,6 +20,3 @@ previous = true domain = ckanext/report/ckanext-report directory = i18n statistics = true - -[tool:pytest] -norecursedirs=ckanext/report/tests/nose diff --git a/setup.py b/setup.py index 67fea7e..56df5ec 100644 --- a/setup.py +++ b/setup.py @@ -27,6 +27,6 @@ tagless_report = ckanext.report.plugin:TaglessReportPlugin [paste.paster_command] - report = ckanext.report.cli.paster_cli:ReportCommand + report = ckanext.report.command:ReportCommand ''', ) diff --git a/test.ini b/test.ini index cbd5cb5..1812f98 100644 --- a/test.ini +++ b/test.ini @@ -10,7 +10,6 @@ port = 5000 [app:main] use = config:../ckan/test-core.ini - ckan.plugins = report tagless_report # Logging configuration From 3d60dc8302e022c621275e4a2a4c36631776592d Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Fri, 19 Aug 2022 14:52:01 +1000 Subject: [PATCH 02/15] [QOL-9161] fix response_view to always return two values - Usually we don't need the second value, but it has to at least be None --- ckanext/report/utils.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/ckanext/report/utils.py b/ckanext/report/utils.py index 9d3e39b..94773e0 100644 --- a/ckanext/report/utils.py +++ b/ckanext/report/utils.py @@ -46,9 +46,9 @@ def report_view(report_name, organization=None, refresh=False): try: report = t.get_action('report_show')({}, {'id': report_name}) except t.NotAuthorized: - return t.abort(401) + return t.abort(401), None except t.ObjectNotFound: - return t.abort(404) + return t.abort(404), None except Exception as e: log.error("Failed to get report: %s", e) raise @@ -56,16 +56,16 @@ def report_view(report_name, organization=None, refresh=False): # ensure correct url is being used if 'organization' in _get_routing_rule()\ and 'organization' not in report['option_defaults']: - return t.redirect_to(helpers.relative_url_for(organization=None)) + return t.redirect_to(helpers.relative_url_for(organization=None)), None elif 'organization' not in _get_routing_rule()\ and 'organization' in report['option_defaults']\ and report['option_defaults']['organization']: org = report['option_defaults']['organization'] - return t.redirect_to(helpers.relative_url_for(organization=org)) + return t.redirect_to(helpers.relative_url_for(organization=org)), None if 'organization' in request.params: # organization should only be in the url - let the param overwrite # the url. - return t.redirect_to(helpers.relative_url_for()) + return t.redirect_to(helpers.relative_url_for()), None # options options = Report.add_defaults_to_options(request.params, report['option_defaults']) @@ -111,21 +111,21 @@ def report_view(report_name, organization=None, refresh=False): try: t.get_action('report_refresh')({}, {'id': report_name, 'options': options}) except t.NotAuthorized: - return t.abort(401) + return t.abort(401), None # Don't want the refresh=1 in the url once it is done - return t.redirect_to(helpers.relative_url_for(refresh=None)) + return t.redirect_to(helpers.relative_url_for(refresh=None)), None # Check for any options not allowed by the report for key in options: if key not in report['option_defaults']: - return t.abort(400, 'Option not allowed by report: %s' % key) + return t.abort(400, 'Option not allowed by report: %s' % key), None try: data, report_date = t.get_action('report_data_get')({}, {'id': report_name, 'options': options}) except t.ObjectNotFound: - return t.abort(404) + return t.abort(404), None except t.NotAuthorized: - return t.abort(401) + return t.abort(401), None if format and format != 'html': ensure_data_is_dicts(data) @@ -134,7 +134,7 @@ def report_view(report_name, organization=None, refresh=False): try: key = t.get_action('report_key_get')({}, {'id': report_name, 'options': options}) except t.NotAuthorized: - return t.abort(401) + return t.abort(401), None filename = 'report_%s.csv' % key response_headers = { 'Content-Type': 'application/csv', @@ -145,7 +145,7 @@ def report_view(report_name, organization=None, refresh=False): data['generated_at'] = report_date return json.dumps(data), {'Content-Type': 'application/json'} else: - return t.abort(400, 'Format not known - try html, json or csv') + return t.abort(400, 'Format not known - try html, json or csv'), None are_some_results = bool(data['table'] if 'table' in data else data) @@ -157,7 +157,7 @@ def report_view(report_name, organization=None, refresh=False): 'report_date': report_date, 'options': options, 'options_html': options_html, 'report_template': report['template'], - 'are_some_results': are_some_results}), {} + 'are_some_results': are_some_results}), None ############################################################################### From 8802e2df01649b91a808e29f18261d125d2316e4 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 10:00:50 +1000 Subject: [PATCH 03/15] [QOL-9161] make pytest more verbose to help debug tests --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5f6d079..c2c40d7 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -61,4 +61,4 @@ jobs: PASTER_PLUGIN=ckanext-report $CKAN_CLI report initdb PASTER_PLUGIN=ckanext-report $CKAN_CLI report generate tagless-datasets - name: Run tests - run: pytest --ckan-ini=test.ini --cov=ckanext.report --disable-warnings ckanext/report/tests + run: pytest -v --ckan-ini=test.ini --cov=ckanext.report --disable-warnings ckanext/report/tests From 471cf1ad09e5169fc33e1e30d7d77f5a77638787 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 12:15:28 +1000 Subject: [PATCH 04/15] [QOL-9161] ditch relative_url_for and use routing properly --- ckanext/report/utils.py | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/ckanext/report/utils.py b/ckanext/report/utils.py index 94773e0..3e31866 100644 --- a/ckanext/report/utils.py +++ b/ckanext/report/utils.py @@ -11,9 +11,8 @@ # CKAN 2.8 from ckan.lib.render import TemplateNotFound import ckan.plugins.toolkit as t -from ckan.plugins.toolkit import request +from ckan.plugins.toolkit import request, url_for -from . import helpers from .lib import make_csv_from_dicts, ensure_data_is_dicts, anonymise_user_names from .report_registry import Report, ReportRegistry @@ -54,18 +53,17 @@ def report_view(report_name, organization=None, refresh=False): raise # ensure correct url is being used - if 'organization' in _get_routing_rule()\ - and 'organization' not in report['option_defaults']: - return t.redirect_to(helpers.relative_url_for(organization=None)), None - elif 'organization' not in _get_routing_rule()\ - and 'organization' in report['option_defaults']\ - and report['option_defaults']['organization']: - org = report['option_defaults']['organization'] - return t.redirect_to(helpers.relative_url_for(organization=org)), None - if 'organization' in request.params: + org_in_route = 'organization' in _get_routing_rule() + org_in_options = report['option_defaults'].get('organization') + if org_in_route and not org_in_options: + return t.redirect_to(url_for('report.view', report_name=report_name)), None + if org_in_options and not org_in_route: + return t.redirect_to(url_for('report.org', report_name=report_name, organization=org_in_options)), None + org_in_params = request.params.get('organization') + if org_in_params: # organization should only be in the url - let the param overwrite # the url. - return t.redirect_to(helpers.relative_url_for()), None + return t.redirect_to(url_for('report.org', report_name=report_name, organization=org_in_params)), None # options options = Report.add_defaults_to_options(request.params, report['option_defaults']) @@ -74,7 +72,7 @@ def report_view(report_name, organization=None, refresh=False): format = options.pop('format') else: format = None - if 'organization' in report['option_defaults']: + if org_in_options: options['organization'] = organization options_html = {} c.options = options # for legacy genshi snippets @@ -113,7 +111,10 @@ def report_view(report_name, organization=None, refresh=False): except t.NotAuthorized: return t.abort(401), None # Don't want the refresh=1 in the url once it is done - return t.redirect_to(helpers.relative_url_for(refresh=None)), None + if organization: + return t.redirect_to(url_for('report.org', report_name=report_name, organization=organization)), None + else: + return t.redirect_to(url_for('report.view', report_name=report_name)), None # Check for any options not allowed by the report for key in options: From faac9f037e974ab28b6d7085971c7aff71dfc5e8 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 14:09:57 +1000 Subject: [PATCH 05/15] [QOL-9161] rename 'list' to avoid clashing with built-in function --- ckanext/report/cli.py | 2 +- ckanext/report/command.py | 2 +- ckanext/report/utils.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ckanext/report/cli.py b/ckanext/report/cli.py index f7436ee..08a0f64 100644 --- a/ckanext/report/cli.py +++ b/ckanext/report/cli.py @@ -40,7 +40,7 @@ def generate(report_list): def list(): """ Lists the reports """ - utils.list() + utils.list_reports() @report.command() diff --git a/ckanext/report/command.py b/ckanext/report/command.py index 8714799..8bf7276 100644 --- a/ckanext/report/command.py +++ b/ckanext/report/command.py @@ -59,7 +59,7 @@ def command(self): if cmd == 'initdb': utils.initdb() elif cmd == 'list': - utils.list() + utils.list_reports() elif cmd == 'generate': report_list = None if len(self.args) == 2: diff --git a/ckanext/report/utils.py b/ckanext/report/utils.py index 3e31866..059e682 100644 --- a/ckanext/report/utils.py +++ b/ckanext/report/utils.py @@ -190,7 +190,7 @@ def generate(report_list): return timings -def list(): +def list_reports(): registry = ReportRegistry.instance() for plugin, report_name, report_title in registry.get_names(): report = registry.get_report(report_name) From f16231413b2af0cfe0c6a3bf1764d18d4984a0b7 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 14:30:54 +1000 Subject: [PATCH 06/15] [QOL-9161] fix redirects - Flask automatically converts query string parameters into routing arguments, so we need to take into account the possibility of having an organisation supplied despite not being on an org URL --- ckanext/report/utils.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/ckanext/report/utils.py b/ckanext/report/utils.py index 059e682..7e165f2 100644 --- a/ckanext/report/utils.py +++ b/ckanext/report/utils.py @@ -53,12 +53,15 @@ def report_view(report_name, organization=None, refresh=False): raise # ensure correct url is being used - org_in_route = 'organization' in _get_routing_rule() - org_in_options = report['option_defaults'].get('organization') - if org_in_route and not org_in_options: - return t.redirect_to(url_for('report.view', report_name=report_name)), None - if org_in_options and not org_in_route: - return t.redirect_to(url_for('report.org', report_name=report_name, organization=org_in_options)), None + if organization or 'organization' in _get_routing_rule(): + if 'organization' not in report['option_defaults']: + # org is supplied but is not a valid input for this report + return t.redirect_to(url_for('report.view', report_name=report_name)), None + else: + org = report['option_defaults'].get('organization') + if org: + # org is not supplied, but this report has a default value + return t.redirect_to(url_for('report.org', report_name=report_name, organization=org)), None org_in_params = request.params.get('organization') if org_in_params: # organization should only be in the url - let the param overwrite @@ -72,7 +75,7 @@ def report_view(report_name, organization=None, refresh=False): format = options.pop('format') else: format = None - if org_in_options: + if organization: options['organization'] = organization options_html = {} c.options = options # for legacy genshi snippets From 046127137d3f1872d63ac2f4a9d54ee836d3321b Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 14:55:22 +1000 Subject: [PATCH 07/15] [QOL-9161] fix tagless query - To generate the query properly, we need to use '== None' rather than 'is None' --- ckanext/report/reports.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckanext/report/reports.py b/ckanext/report/reports.py index 49dc7c0..aa45db5 100644 --- a/ckanext/report/reports.py +++ b/ckanext/report/reports.py @@ -31,7 +31,7 @@ def tagless_report(organization, include_sub_organizations=False): # Find the packages without tags q = model.Session.query(model.Package) \ .outerjoin(model.PackageTag) \ - .filter(model.PackageTag.id is None) + .filter(model.PackageTag.id == None) # noqa: E711 if organization: q = lib.filter_by_organizations(q, organization, include_sub_organizations) From 20aa01f1fc1f32b138cfbdf312a5153c5e1c7fea Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 15:00:24 +1000 Subject: [PATCH 08/15] [QOL-9161] fix helper definition - pass the function, not the result of it --- ckanext/report/plugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckanext/report/plugin.py b/ckanext/report/plugin.py index fd40b48..a521206 100644 --- a/ckanext/report/plugin.py +++ b/ckanext/report/plugin.py @@ -34,7 +34,7 @@ def get_helpers(self): 'report__organization_list': h.organization_list, 'report__render_datetime': h.render_datetime, 'report__explicit_default_options': h.explicit_default_options, - 'is_ckan_29': h.is_ckan_29(), + 'is_ckan_29': h.is_ckan_29, } # IActions From 3b336c3566929e4789a4fd11632c962dbb1f9d11 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 15:11:06 +1000 Subject: [PATCH 09/15] [QOL-9161] fix test expectations - don't assume that a newly added dataset will appear in the report when we haven't refreshed it --- ckanext/report/tests/test_plugin.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ckanext/report/tests/test_plugin.py b/ckanext/report/tests/test_plugin.py index 307a768..1f54c07 100644 --- a/ckanext/report/tests/test_plugin.py +++ b/ckanext/report/tests/test_plugin.py @@ -45,12 +45,10 @@ def test_tagless_report_listed(self, app): def test_tagless_report(self, app): u"""Test tagless report generation""" - dataset = factories.Dataset() - res = app.get(u'/report/tagless-datasets') _assert_in_body(u"Datasets which have no tags.", res) - _assert_in_body('href="/dataset/' + dataset['name'] + '"', res) + _assert_in_body('

    Results

    ', res) def test_tagless_report_csv(self, app): u"""Test tagless report generation""" From 0b2b2f8cc60d9c603a15c4beee268a08ecdd0d32 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 15:22:42 +1000 Subject: [PATCH 10/15] [QOL-9161] fix breadcrumb links - Explicitly define index route so Py3 is happy - Add formatting so breadcrumbs are pretty --- ckanext/report/blueprint.py | 2 +- ckanext/report/templates/report/index.html | 2 +- ckanext/report/templates/report/view.html | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ckanext/report/blueprint.py b/ckanext/report/blueprint.py index 35fd82c..8695fbe 100644 --- a/ckanext/report/blueprint.py +++ b/ckanext/report/blueprint.py @@ -27,7 +27,7 @@ def view(report_name, organization=None, refresh=False): return body -report.add_url_rule(u'/report', view_func=index) +report.add_url_rule(u'/report', 'index', view_func=index) report.add_url_rule(u'/reports', 'reports', view_func=redirect_to_index) report.add_url_rule(u'/report/', view_func=view, methods=('GET', 'POST')) report.add_url_rule(u'/report//', 'org', view_func=view, methods=('GET', 'POST',)) diff --git a/ckanext/report/templates/report/index.html b/ckanext/report/templates/report/index.html index 46b77e1..0433dc0 100644 --- a/ckanext/report/templates/report/index.html +++ b/ckanext/report/templates/report/index.html @@ -3,7 +3,7 @@ {% block title %}{{ _('Reports') }} - {{ super() }}{% endblock %} {% block breadcrumb_content %} - {{ h.nav_link(_('Reports'), named_route='report.index') }} +
  • {{ h.nav_link(_('Reports'), named_route='report.index') }}
    • {% endblock %} {% block primary_content_inner %} diff --git a/ckanext/report/templates/report/view.html b/ckanext/report/templates/report/view.html index f700100..adf33b2 100644 --- a/ckanext/report/templates/report/view.html +++ b/ckanext/report/templates/report/view.html @@ -3,8 +3,8 @@ {% block title %}{{ report.title }} - {{ _('Reports') }} - {{ super() }}{% endblock %} {% block breadcrumb_content %} - {{ h.nav_link(_('Reports'), named_route='report.index') }} - {{ h.nav_link(report.title, named_route='report.org' if '/organization' in request.environ.get('PATH_INFO', '') else 'report.view', report_name=report_name) }} +
  • {{ h.nav_link(_('Reports'), named_route='report.index') }}
    • +
  • {{ h.nav_link(report.title, named_route='report.org' if '/organization' in request.environ.get('PATH_INFO', '') else 'report.view', report_name=report_name) }}
    • {% endblock%} {% block primary_content_inner %} From 95295285c884a9e3c813c8d7b20ccc5ef0017b60 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 29 Aug 2022 15:30:44 +1000 Subject: [PATCH 11/15] [QOL-9161] drop obsolete Tablesorter --- README.md | 2 +- .../templates/report/tagless-datasets.html | 2 +- ckanext/report/templates/report/view.html | 94 +++++++++---------- 3 files changed, 46 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 97d63e3..73bbe53 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,7 @@ data - other data values, as a dict
  • Average tags per package: {{ data['average_tags_per_package'] }} tags
- +
diff --git a/ckanext/report/templates/report/tagless-datasets.html b/ckanext/report/templates/report/tagless-datasets.html index 4e0d4b6..5444ab6 100644 --- a/ckanext/report/templates/report/tagless-datasets.html +++ b/ckanext/report/templates/report/tagless-datasets.html @@ -11,7 +11,7 @@
  • {% trans %}Average tags per package{% endtrans %}: {{ data['average_tags_per_package'] }} tags
    • -
    Dataset
    +
    diff --git a/ckanext/report/templates/report/view.html b/ckanext/report/templates/report/view.html index adf33b2..ddc4833 100644 --- a/ckanext/report/templates/report/view.html +++ b/ckanext/report/templates/report/view.html @@ -8,67 +8,61 @@ {% endblock%} {% block primary_content_inner %} -

    {{ report.title }}

    -

    {{ report.description }}

    -

    - {{ _('Generated') }}: {{ h.report__render_datetime(report_date, '%d/%m/%Y %H:%M') }} -

    - {% if c.userobj.sysadmin %} -
    -
    {% trans %}Refresh report{% endtrans %}
    -
    -
    - - -

    {{ _('As a system administrator you are able to refresh this report on demand by clicking the \'Refresh\' button.') }}

    -
    +

    {{ report.title }}

    +

    {{ report.description }}

    +

    + {{ _('Generated') }}: {{ h.report__render_datetime(report_date, '%d/%m/%Y %H:%M') }} +

    + {% if c.userobj.sysadmin %} +
    +
    {% trans %}Refresh report{% endtrans %}
    +
    +
    + + +

    {{ _('As a system administrator you are able to refresh this report on demand by clicking the \'Refresh\' button.') }}

    - {% endif %} +
    + {% endif %} - {% if options %} -

    {{ _('Options') }}

    -
    - {% for key, value in options.items() %} - {% if key in options_html %} - {{ options_html[key]|safe }} - {% else %} - {{ key }}: {{ value }} - - {% endif %} -
    - {% endfor %} - - {% endif %} + {% if options %} +

    {{ _('Options') }}

    +
    + {% for key, value in options.items() %} + {% if key in options_html %} + {{ options_html[key]|safe }} + {% else %} + {{ key }}: {{ value }} + + {% endif %} +
    + {% endfor %} + + {% endif %} - {% if are_some_results %} -
    - {{ _('Download') }}: - CSV - JSON -
    - {% endif %} -

    {{ _('Results') }}

    - {% if not are_some_results %} -

    {{ _('No results found.') }}

    - {% else %} -
    - {% snippet report_template, table=data['table'], data=data, report_name=report_name, options=options %} -
    - {% endif %} -
    + {% if are_some_results %} +
    + {{ _('Download') }}: + CSV + JSON +
    + {% endif %} +

    {{ _('Results') }}

    + {% if not are_some_results %} +

    {{ _('No results found.') }}

    + {% else %} +
    + {% snippet report_template, table=data['table'], data=data, report_name=report_name, options=options %} +
    + {% endif %} {% endblock%} {% block scripts %} {{ super() }} - -
    {% trans %}Dataset{% endtrans %}