accessModeSufficient check is broken, as it will not accept a list of more than two comma-separated values

[JohnThomson]

Ace version 1.1.1 Node version 12.14.0 Windows 10

For example, if content.opf contains textual,visual,auditory</opf:meta> the ACE json report contains "assertions": [ { "@type": "earl:assertion", "earl:result": { "earl:outcome": "fail", "dct:description": "'schema:accessModeSufficient' metadata must be set to one of the expected values" }, "earl:assertedBy": "Ace", "earl:mode": "automatic", "earl:test": { "earl:impact": "moderate", "dct:title": "metadata-accessmodesufficient-invalid", "dct:description": "Value 'visual,auditory' is invalid for 'schema:accessModeSufficient' metadata", "help": { "url": "http://kb.daisy.org/publishing/docs/metadata/schema-org.html", "dct:title": "Schema.org Accessibility Metadata", "dct:description": "Use one of the metadata values defined by schema.org" }, "rulesetTags": [ "EPUB" ] } } ], Note that it has evaluated the 2nd and 3rd elements in the list as a single item "visual,auditory"

I found the problem on line 78 of checker-epub.js, where replace(',', ' ') only replaces the first comma. It should be replace(/,/g, ' ')

The attached zip is actually a complete epub. Not a minimal demonstration, but it does produce the error.

Hoping to submit a PR shortly...

Atchoum !-français.zip

[JohnThomson]

https://github.com/daisy/ace/pull/332 should fix this.

Note: I tested it on the attached book, but was unable to get unit tests to run with or without the proposed change to test data. "yarn test" (after yarn and yarn install) just complains that " [31724]: c:\ws\src\node_file.cc:1337: Assertion `(argc) >= (3)' failed." and I can't figure out what more arguments it wants.

[danielweck]

Thank you for reporting this bug and for proposing a fix.

However, I believe this problem - as well as a few others - was fixed some time ago in the "next" version of Ace, currently distributed as release 1.2.0-beta.12 (note that although it is labelled as "beta", it actually ships in Ace App which appears to be satisfactorily stable, see https://github.com/daisy/ace-gui/releases )

NPM package tagged as next: https://www.npmjs.com/package/@daisy/ace/v/next (note that Ace version 1.1.1 is published under the latest NPM tag: https://www.npmjs.com/package/@daisy/ace/v/latest )

Pending Pull Request, which will supersede the master branch / version 1.1.1: https://github.com/daisy/ace/pull/314 ...associated ace-next branch: https://github.com/daisy/ace/tree/ace-next

Would you mind running your test against Ace version 1.2.0-beta.12? You can install it globally using npm -g install ace@next or locally in your project (i.e. node_modules folder) or using a temporary execution with npx ace@next {COMMAND_LINE_PARAMS}.

[danielweck]

For future reference, here is the fix that ships in 1.2.0-beta.12:

https://github.com/daisy/ace/blob/a6fbed40476f4dfce49a563d0f9a8db7c28fa4ae/packages/ace-core/src/checker/checker-epub.js#L103-L109

...versus:

https://github.com/daisy/ace/pull/332/files#diff-998b7bddbd544ebbd20af762228e4167L162-L164

[danielweck]

Also note the associated unit tests:

https://github.com/daisy/ace/blob/a6fbed40476f4dfce49a563d0f9a8db7c28fa4ae/tests/__tests__/epub-rules.test.js#L57-L152

https://github.com/daisy/ace/blob/a6fbed40476f4dfce49a563d0f9a8db7c28fa4ae/tests/data/epubrules-metadata-accessModeSufficient-invalid-item/EPUB/package.opf#L8-L14

https://github.com/daisy/ace/blob/a6fbed40476f4dfce49a563d0f9a8db7c28fa4ae/tests/data/epubrules-metadata-accessModeSufficient-invalid-separator/EPUB/package.opf#L8-L14

https://github.com/daisy/ace/blob/a6fbed40476f4dfce49a563d0f9a8db7c28fa4ae/tests/data/epubrules-metadata-accessModeSufficient-missing/EPUB/package.opf#L8-L13

https://github.com/daisy/ace/blob/a6fbed40476f4dfce49a563d0f9a8db7c28fa4ae/tests/data/epubrules-metadata-accessModeSufficient-valid/EPUB/package.opf#L8-L16

https://github.com/daisy/ace/blob/a6fbed40476f4dfce49a563d0f9a8db7c28fa4ae/tests/data/epubrules-metadata-accessibilityFeature-case-sensitive/EPUB/package.opf#L8-L16

[JohnThomson]

Yes, the next version works correctly. (I found to install it I actually needed npm install -g @daisy/ace@next).

Any idea when this will be stable? The instructions for installing Ace by DAISY ship as part of our program (bloom, bloomlibrary.org), and while your 'next' branch may be stable enough to recommend to our users right now, will it always be?

[danielweck]

Yes, the "next" branch and corresponding NPM version (currently '1.2.0-beta.12') will either continue to receive updates (as part of "beta" or "release candidate" builds), or will become the main release channel in order to supersede/deprecate v'1.1.1' / current "master" branch. The "next" branch is effectively the active development channel for Ace and Ace App releases. There have been occasional code fixes directly in master, but "next" is guaranteed to include all of "master"'s history. We have been Keeping "master" alive because of initial concerns for supporting legacy / old NodeJS runtimes (which "next" doesn't support anymore, due to alignment with LTS Long Term Support NodeJS versions). We were also cautious (perhaps over cautious) to ensure sufficient field testing of Ace "beta" via the Ace App releases, and the odd 1.1 user interested in using the 1.2 release stream. We are now pretty confident that it is time to elevate 1.2 to the status of stable, and thereby make 1.1 obsolete. I anticipate this to happen by the end of the year.

[danielweck]

ace-next will be published shortly to replace Ace 1.1.1 https://github.com/daisy/ace/pull/314

This issue can now be closed.