Some feedback from Victor on misc small improvements (Sep 2019)

[rufuspollock]

DMS

Structure

The entire DMS sits in one file, importing a model from the same file and a utils lib from another file. The CKAN API is too vast to be contained in a flat structure, so breaking the DMS / model into namespaces/endpoints.

Example:

lib
└── dms.js
utils
└── index.js

I accidentally reimplemented the DMS over the weekend, not knowing it exists. Proposed structure is something around these lines (snippets truncated):

lib
└── ckan
    ├── api
    │   ├── group.js
    │   ├── index.js
    │   └── package.js
    ├── index.js
    └── models.js

With a usage like:

const ckan = require('./plugins/ckan')

module.exports = function (app) {
  app.get('/', async (req, res) => {
    const packageApi = new ckan.api.package();
    const groupApi = new ckan.api.group();

    let featured = []
    await packageApi.search({
      query: '', 
      fq: '',   // optional
      rows: 10, // optional
      from: 0,  // optional
      sort: 'metadata_modified desc' // optional
    })

[...]

This not only aims to map 1:1 to CKAN API features, thus making it more intuitive, but also makes use of the more extensive documentation for endpoints and their params.

Functionality

Not all of the CKAN API endpoins features are implemented. Normaly this wouldn't be a problem, but we're having issues where custom provided parameters are discarded

For example: the package_search endpoint is only forwarding a predefined, hardcoded structure to the API and we had to patch it to support the fq param for Solr.

Bizarre naming conventions

getPackage - gets a package
search - searches packages
getOrganizations - gets all organizations
getOwner - gets a specific organization??
getCollection - hits group_show??

In search there's a utils.convertToCkanSearchQuery(query) that accepts, among others:

• a size parameter that actually sends a rows search param
• a start param that sends from
  These renames are not documented and their rationale is not explained anywhere. They are also not obvious.

Purpose / overlaps existing libraries

Not a real issue we've been having in Dept Ed, but the DMS seems to be a CKAN API adapter. Why aren't we using, for example, the okfn/ckan.js project? (while that might be outdated, could be easier to update it for working with more modern CKAN than reimplementing a slightly different version of it from scratch). Bonus, it has Recline features.

Theming

Extending core libs outside the scope of a theme

The recommended approach for adding functionality for the DMS is, as I understand, to extend it in a theme that is loaded as a plugin, therefore overloading the original module and possibly overriding some of its functions.

While this is not necessarily bad (we do this for classic CKAN already), some of the features that currently need to be extended provide basic functionality of a default CKAN frontend and they should be baked in the main lib.

Example: in Roche theme, there is an extended DMS version that adds 'Authorization' headers to the search function. Authenticating and authorizing requests should be part of the DMS.

Extending templates kinda circles back to classic CKAN / Jinja

There is an issue in frontend-v2 GitHub repository that aims at making (and maybe implementing) a spec for template inheritance chain. This has the potential of eliminating one of the key benefits of NG, of holding the templates together and easier to design / develop / adjust. If we go back to snippets and templates broken into many pieces, where is the gain?

Boilerplate that should land in utils module - to collect from projects

There might be some boilerplate functions that could prove useful in many scenarios and might be worth collecting from projects using the NG frontend already. For example, in Dept Ed we have some URL related functions that are not really that specific:

• adjust (add, remove, update) query string params - helpers for generating URLs in templates
• construct a package_search query based on the query string

Mocking

One big monolithic file

Mocking is easy to implement, but some mocked values are huge and the file (the ONE file) holding them is already getting too big. We should refactor the mocks and split by namespace, endpoint or something like that.

Maybe a structure like:

fixtures
└──items
│  └──packages.js
│  └──groups.js
│  └──organizations.js
└──index.js
    # this file could hold the endpoints, include 
    # the mocked items and combine them as necessary 
    # instead of duplicating resources by mocking 
    # entire values as strings

Should mock per-route instead of per-host

Mocking is good, but if I develop a theme and either:

• I have an endpoint that has no mocks (yet) OR
• I want to test one endpoint against a local CKAN instance running on localhost:5000

Then my only choice as a developer is to either enable or disable mocking for the entire host.