Merge pull request #139 from open-data/feature/field-errors-n-refresh

JVickery-TBS · web-flow · commit f266b19bdba3 · 2025-02-10T15:29:21.000-05:00
Feature/field errors n refresh
diff --git a/changes/139.changes b/changes/139.changes
@@ -0,0 +1 @@
+Now gracefully handles browser page refreshes on `POST` views.
diff --git a/changes/139.feature b/changes/139.feature
@@ -0,0 +1 @@
+Now handles `KeyError` and `fields` errors when downloading and uploading Excel records, prompting the user to refresh the resource.
diff --git a/ckanext/recombinant/cli.py b/ckanext/recombinant/cli.py
@@ -122,20 +122,27 @@ def remove_empty(dataset_type: Optional[List[str]] = None,
     is_flag=True,
     help="Deletes fields that are no longer in the Schema (requires --force-update)",
 )
+@click.option(
+    "-D",
+    "--dataset",
+    help="A dataset ID to update all resource tables for."
+)
 @click.option('-v', '--verbose', is_flag=True,
               type=click.BOOL, help='Increase verbosity.')
 def update(dataset_type: Optional[List[str]] = None,
            all_types: bool = False,
            force_update: bool = False,
            delete_fields: bool = False,
+           dataset: Optional[str] = None,
            verbose: bool = False):
     """
     Triggers recombinant update for recombinant resources
 
     Full Usage:\n
         recombinant update (-a | DATASET_TYPE ...) [-f]
     """
-    _update(dataset_type, all_types, force_update, delete_fields, verbose=verbose)
+    _update(dataset_type, all_types, force_update, delete_fields,
+            dataset_id=dataset, verbose=verbose)
 
 
 @recombinant.command(short_help="Delete recombinant datasets and all their data.")
@@ -357,12 +364,26 @@ def _update(dataset_types: Optional[List[str]],
             all_types: bool = False,
             force_update: bool = False,
             delete_fields: bool = False,
+            dataset_id: Optional[str] = None,
             verbose: bool = False):
     """
     Triggers recombinant update for recombinant resources
     """
     orgs = _get_orgs()
     lc = LocalCKAN()
+    if dataset_id:
+        dataset_dict = lc.action.package_show(id=dataset_id)
+        click.echo('%s %s updating single dataset %s' % (
+            dataset_dict['type'],
+            dataset_dict['organization']['name'],
+            dataset_id))
+        lc.action.recombinant_update(
+            owner_org=dataset_dict['organization']['name'],
+            dataset_type=dataset_dict['type'],
+            dataset_id=dataset_id,
+            force_update=force_update,
+            delete_fields=delete_fields)
+        return
     for dtype in _expand_dataset_types(dataset_types, all_types):
         packages = _get_packages(dtype, orgs)
         existing = dict((p['owner_org'], p) for p in packages)
diff --git a/ckanext/recombinant/errors.py b/ckanext/recombinant/errors.py
@@ -7,6 +7,10 @@ class RecombinantException(Exception):
     pass
 
 
+class RecombinantFieldError(Exception):
+    pass
+
+
 class RecombinantConfigurationError(Exception):
     pass
 
diff --git a/ckanext/recombinant/logic.py b/ckanext/recombinant/logic.py
@@ -59,6 +59,7 @@ def recombinant_update(context: Context, data_dict: DataDict):
     recombinant dataset type.
 
     :param dataset_type: recombinant dataset type
+    :param dataset_id: dataset id to update specifically for
     :param owner_org: organization name or id
     :param delete_resources: True to delete extra resources found
     :param force_update: True to force updating of datastore tables
@@ -119,6 +120,8 @@ def recombinant_show(context: Context, data_dict: DataDict) -> Dict[str, Any]:
                 limit=0)
             datastore_correct = _datastore_match(r['fields'], ds['fields'])
             out['datastore_correct'] = datastore_correct
+            schema_correct = _schema_match(r['fields'], ds['fields'])
+            out['schema_correct'] = schema_correct
             resources_correct = resources_correct and datastore_correct
             out['datastore_rows'] = ds.get('total', 0)
             out['datastore_active'] = True
@@ -150,6 +153,7 @@ def _action_find_dataset(context: Context, data_dict: DataDict) -> Tuple[
     '''
     dataset_type = get_or_bust(data_dict, 'dataset_type')
     owner_org = Group.get(get_or_bust(data_dict, 'owner_org'))
+    dataset_id = data_dict.get('dataset_id', None)
 
     if not owner_org:
         raise ValidationError(
@@ -167,7 +171,9 @@ def _action_find_dataset(context: Context, data_dict: DataDict) -> Tuple[
 
     lc = LocalCKAN(username=context['user'], context=fresh_context)
     result = lc.action.package_search(
-        q="type:%s AND organization:%s" % (dataset_type, owner_org.name),
+        q="type:%s AND organization:%s %s" % (
+            dataset_type, owner_org.name,
+            'AND id:{0}'.format(dataset_id) if dataset_id else ''),
         include_private=True,
         rows=2)
     return lc, geno, result['results']
@@ -433,6 +439,17 @@ def _datastore_match(fs: List[Dict[str, Any]], fields: List[Dict[str, Any]]) ->
                if not f.get('published_resource_computed_field', False))
 
 
+def _schema_match(fs: List[Dict[str, Any]], fields: List[Dict[str, Any]]) -> bool:
+    """
+    return True if datastore column fields are all fields defined in fs.
+    """
+    # XXX: does not check types or extra columns at this time
+    existing = set(f['datastore_id'] for f in fs
+                   if not f.get('published_resource_computed_field', False))
+    return all(c['id'] in existing for c in fields
+               if c['id'] != '_id')
+
+
 @chained_action
 @side_effect_free
 def recombinant_datastore_info(up_func: Action,
diff --git a/ckanext/recombinant/templates/recombinant/resource_edit.html b/ckanext/recombinant/templates/recombinant/resource_edit.html
@@ -45,7 +45,7 @@
   {% block action_panels %}
     {% if dataset %}
       {% if 'error' not in resource %}
-        {% if g.userobj.sysadmin and not resource.datastore_correct %}
+        {% if g.userobj.sysadmin and (not resource.datastore_correct or not resource.schema_correct) %}
           {# only let sysadmins refresh the recombinant record via UI #}
           <div class="module-alert alert alert-warning">
             <h3>{{ _("The Recombinant resource is out of date") }}{% snippet 'snippets/sysadmin_only.html' %}</h3>
diff --git a/ckanext/recombinant/templates/recombinant/snippets/outdated_error.html b/ckanext/recombinant/templates/recombinant/snippets/outdated_error.html
@@ -0,0 +1,21 @@
+<h3>{{_('Errors in Recombinant Resource')}}</h3>
+<p>{{ _('The Recombinant resource is out of date:') }}</p>
+<ul>
+    <li>{{ _('Field(s) <em>{0}</em> do not exist in the database for this resource.').format(key_errors) }}</li>
+</ul>
+{% if not g.userobj.sysadmin %}
+  <p>{{ _('Please contact <a href="mailto:open-ouvert@tbs-sct.gc.ca">open-ouvert@tbs-sct.gc.ca</a> for assistance.') }}</p>
+{% else %}
+  {# only let sysadmins refresh the recombinant record via UI #}
+  <div class="clearfix"></div>
+  <h3>{{ _('Refresh Recombinant Resource') }}{% snippet 'snippets/sysadmin_only.html' %}</h3>
+  <p>{{ _('You can refresh your resource in the database to try to solve the problem.') }}</p>
+  <div class="clearfix"></div>
+  <p class="form-actions">
+    <form id="recombinant-refresh-form" action="{{ h.url_for('recombinant.refresh_dataset', resource_name=res_name, owner_org=owner_org) }}" method="post">
+      {{ h.csrf_input() }}
+      <input type="hidden" name="dataset_id" value="{{ dataset_id }}"/>
+      <button class="btn btn-danger mrgn-bttm-md m-b-3" type="submit" name="refresh" >{{ _('Refresh…') }}</button>
+    </form>
+  </p>
+{% endif %}
diff --git a/ckanext/recombinant/views.py b/ckanext/recombinant/views.py
@@ -32,6 +32,7 @@
 
 from ckanext.recombinant.errors import (
     RecombinantException,
+    RecombinantFieldError,
     BadExcelData,
     format_trigger_error
 )
@@ -59,17 +60,25 @@
 from ckanapi import NotFound, LocalCKAN
 
 
+KEY_ERROR_MATCH = re.compile('"([^"]*)"')
+
 log = getLogger(__name__)
 recombinant = Blueprint('recombinant', __name__)
 
 
-@recombinant.route('/recombinant/upload/<id>', methods=['POST'])
-def upload(id: str) -> Response:
+@recombinant.route('/recombinant/upload/<id>', methods=['GET', 'POST'])
+def upload(id: str) -> Union[Response, str]:
     package_type = _get_package_type(id)
     geno = get_geno(package_type)
     lc = LocalCKAN(username=g.user)
     dataset = lc.action.package_show(id=id)
     org = lc.action.organization_show(id=dataset['owner_org'])
+    if request.method != 'POST':
+        # handle page refreshes
+        h.flash_notice(_('Form not submitted, please try again.'))
+        return h.redirect_to('recombinant.preview_table',
+                             resource_name=package_type,
+                             owner_org=org['name'])
     dry_run = 'validate' in request.form
     # resource_name is only required for redirect,
     # so do not need to heavily validate that it exists in the geno.
@@ -98,6 +107,15 @@ def upload(id: str) -> Response:
         return h.redirect_to('recombinant.preview_table',
                              resource_name=resource_name,
                              owner_org=org['name'])
+    except RecombinantFieldError as e:
+        h.flash_error(render('recombinant/snippets/outdated_error.html',
+                             extra_vars={'key_errors': str(e).replace("'", ''),
+                                         'dataset_id': dataset['id'],
+                                         'res_name': resource_name,
+                                         'owner_org': dataset['owner_org']}))
+        return h.redirect_to('recombinant.preview_table',
+                             resource_name=resource_name,
+                             owner_org=org['name'])
     except BadExcelData as e:
         h.flash_error(_(e.message))
         return h.redirect_to('recombinant.preview_table',
@@ -120,6 +138,13 @@ def delete_records(id: str, resource_id: str) -> Union[str, Response]:
     res = lc.action.resource_show(id=resource_id)
     org = lc.action.organization_show(id=pkg['owner_org'])
 
+    if request.method != 'POST':
+        # handle page refreshes
+        h.flash_notice(_('Form not submitted, please try again.'))
+        return h.redirect_to('recombinant.preview_table',
+                             resource_name=res['name'],
+                             owner_org=org['name'])
+
     dataset = lc.action.recombinant_show(
         dataset_type=pkg['type'], owner_org=org['name'])
 
@@ -190,7 +215,8 @@ def record_fail(err: str) -> str:
             'recombinant.preview_table',
             resource_name=res['name'],
             owner_org=org['name'])
-    if 'confirm' not in request.form or request.method == 'GET':
+    # type_ignore_reason: incomplete typing
+    if 'confirm' not in request.form or request.method == 'GET':  # type: ignore
         return render('recombinant/confirm_delete.html',
                       extra_vars={'dataset': dataset,
                                   'resource': res,
@@ -249,7 +275,7 @@ def _xlsx_response_headers() -> Tuple[str, str]:
 
 @recombinant.route('/recombinant-template/<dataset_type>_<lang>_<owner_org>.xlsx',
                    methods=['GET', 'POST'])
-def template(dataset_type: str, lang: str, owner_org: str) -> Response:
+def template(dataset_type: str, lang: str, owner_org: str) -> Union[Response, str]:
     """
     POST requests to this endpoint contain primary keys of
     records that are to be included in the excel file
@@ -307,7 +333,17 @@ def template(dataset_type: str, lang: str, owner_org: str) -> Response:
                              resource['id'])
             record_data += result['records']
 
-        append_data(book, record_data, chromo)
+        try:
+            append_data(book, record_data, chromo)
+        except RecombinantFieldError as e:
+            h.flash_error(render('recombinant/snippets/outdated_error.html',
+                                 extra_vars={'key_errors': str(e).replace("'", ''),
+                                             'dataset_id': dataset['id'],
+                                             'res_name': resource_name,
+                                             'owner_org': dataset['owner_org']}))
+            return h.redirect_to('recombinant.preview_table',
+                                 resource_name=resource_name,
+                                 owner_org=org['name'])
 
         resource_names = dict((r['id'], r['name']) for r in dataset['resources'])
         ds_info = lc.action.datastore_info(id=resource['id'])
@@ -576,7 +612,8 @@ def preview_table(resource_name: str,
             # check that the resource has errors
             for _r in dataset['resources']:
                 if _r['name'] == resource_name and ('error' in _r or
-                                                    not _r['datastore_correct']):
+                                                    not _r['datastore_correct'] or
+                                                    not _r['schema_correct']):
                     raise NotFound
         except NotFound:
             try:
@@ -632,6 +669,50 @@ def preview_table(resource_name: str,
         })
 
 
+@recombinant.route('/recombinant/refresh/<resource_name>/<owner_org>',
+                   methods=['GET', 'POST'])
+def refresh_dataset(resource_name: str, owner_org: str):
+    if not is_sysadmin(g.user):
+        return abort(403)
+    if request.method != 'POST':
+        # handle page refreshes
+        h.flash_notice(_('Form not submitted, please try again.'))
+        return h.redirect_to('recombinant.preview_table',
+                             resource_name=resource_name,
+                             owner_org=owner_org)
+    if 'refresh' in request.form:
+        dataset_id = request.form['dataset_id']
+        if not dataset_id:
+            h.flash_error(_('Could not determine dataset to update.'))
+        else:
+            lc = LocalCKAN()
+            dataset_dict = lc.action.package_show(id=dataset_id)
+            for res in dataset_dict['resources']:
+                if not h.check_access('datastore_delete',
+                                      {'resource_id': res['id'],
+                                       'filters': {}}):
+                    return abort(403, _('User {0} not authorized '
+                                        'to update resource {1}'.format(
+                                            str(g.user), res['id'])))
+            try:
+                lc.action.recombinant_update(
+                    owner_org=dataset_dict['organization']['name'],
+                    dataset_type=dataset_dict['type'],
+                    dataset_id=dataset_id,
+                    force_update=True)
+                h.flash_success(_('Resources successfully refreshed.'))
+            except Exception:
+                h.flash_error(_(
+                    'Unable to regenerate the resources. Please contact '
+                    '<a href="mailto:open-ouvert@tbs-sct.gc.ca">'
+                    'open-ouvert@tbs-sct.gc.ca</a> for assistance.'),
+                    allow_html=True)
+    return h.redirect_to(
+        'recombinant.preview_table',
+        resource_name=resource_name,
+        owner_org=owner_org)
+
+
 def _process_upload_file(lc: LocalCKAN,
                          dataset: Dict[str, Any],
                          upload_file: Union[str, FlaskFileStorage, FieldStorage],
@@ -752,6 +833,15 @@ def _process_upload_file(lc: LocalCKAN,
                     # type_ignore_reason: incomplete typing
                     pgerror = e.error_dict['info']['orig'][0].decode(  # type: ignore
                         'utf-8')
+                elif 'fields' in e.error_dict:
+                    # type_ignore_reason: incomplete typing
+                    pgerror = e.error_dict['fields'][0]  # type: ignore
+                    key = re.search(KEY_ERROR_MATCH, str(pgerror))
+                    if key:
+                        key = key.group(1)
+                    else:
+                        key = _('unknown')
+                    raise RecombinantFieldError(key)
                 else:
                     # type_ignore_reason: incomplete typing
                     pgerror = e.error_dict['records'][0]  # type: ignore
diff --git a/ckanext/recombinant/write_excel.py b/ckanext/recombinant/write_excel.py
@@ -21,7 +21,7 @@
 from typing import Any, Dict, List, Tuple, Optional, Union
 
 from ckanext.recombinant.tables import get_geno
-from ckanext.recombinant.errors import RecombinantException
+from ckanext.recombinant.errors import RecombinantException, RecombinantFieldError
 from ckanext.recombinant.datatypes import datastore_type
 from ckanext.recombinant.helpers import (
     recombinant_choice_fields, recombinant_language_text)
@@ -32,7 +32,7 @@
 from datetime import datetime
 from decimal import Decimal
 
-
+SHEET_PROTECTION = True
 DEFAULT_TEMPLATE_VERSION = 3
 
 HEADER_ROW, HEADER_HEIGHT = 1, 27
@@ -132,7 +132,7 @@ def excel_template(dataset_type: str,
             _append_resource_ref_header(geno, refs, rnum)
             choice_ranges.append(_populate_excel_sheet(
                 book, sheet, geno, chromo, org, refs, rnum))
-            sheet.protection.enabled = True
+            sheet.protection.enabled = SHEET_PROTECTION
             sheet.protection.formatRows = False
             sheet.protection.formatColumns = False
         # type_ignore_reason: incomplete typing
@@ -141,7 +141,7 @@ def excel_template(dataset_type: str,
     if version == 3:
         _populate_reference_sheet(sheet, geno, refs)
     sheet.title = 'reference'
-    sheet.protection.enabled = True
+    sheet.protection.enabled = SHEET_PROTECTION
 
     if version == 2:
         return book
@@ -152,13 +152,13 @@ def excel_template(dataset_type: str,
         sheet: Worksheet = book.create_sheet()  # type: ignore
         _populate_excel_e_sheet(sheet, chromo, cranges)
         sheet.title = 'e{i}'.format(i=i)
-        sheet.protection.enabled = True
+        sheet.protection.enabled = SHEET_PROTECTION
         sheet.sheet_state = 'hidden'
         # type_ignore_reason: incomplete typing
         sheet: Worksheet = book.create_sheet()  # type: ignore
         _populate_excel_r_sheet(sheet, chromo)
         sheet.title = 'r{i}'.format(i=i)
-        sheet.protection.enabled = True
+        sheet.protection.enabled = SHEET_PROTECTION
         sheet.sheet_state = 'hidden'
     return book
 
@@ -175,6 +175,8 @@ def append_data(book: Workbook,
     current_row = DATA_FIRST_ROW
     for record in record_data:
         for col_num, field in template_cols_fields(chromo):
+            if field['datastore_id'] not in record:
+                raise RecombinantFieldError(field['datastore_id'])
             item = datastore_type_format(
                 record[field['datastore_id']], field['datastore_type'])
             sheet.cell(row=current_row, column=col_num).value = item

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Now gracefully handles browser page refreshes on `POST` views.
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Now handles `KeyError` and `fields` errors when downloading and uploading Excel records, prompting the user to refresh the resource.