Hello jenkins-bot, Mholloway,
I'd like you to do a code review. Please visit
https://gerrit.wikimedia.org/r/398183
to review the following change.
Change subject: Revert "Revert "Update section validation logic""
......................................................................
Revert "Revert "Update section validation logic""
This reverts commit ddddebb87374b3c69ff6a3a71dffaf4e1e64af9b.
It's easier to see what has changed from the previous solution
if we split up the changes into two commits. I'm also going to add
a unit test for the log call.
Change-Id: I2394ee5d14b7cb83f329c7f17e735a8f8d6342f9
---
M lib/parsoid-access.js
M lib/parsoidSections.js
M lib/parsoidSectionsUsingSectionTags.js
M test/lib/parsoid/parsoid-sections-section-elements-tests.js
4 files changed, 57 insertions(+), 31 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/services/mobileapps
refs/changes/83/398183/1
diff --git a/lib/parsoid-access.js b/lib/parsoid-access.js
index 338ce33..3ae3308 100644
--- a/lib/parsoid-access.js
+++ b/lib/parsoid-access.js
@@ -119,7 +119,7 @@
transforms.stripUnneededMarkup(doc, legacy);
parsoidSections.addSectionDivs(doc);
- page.sections = parsoidSections.getSectionsText(doc);
+ page.sections = parsoidSections.getSectionsText(doc, req.logger);
return page;
});
}
diff --git a/lib/parsoidSections.js b/lib/parsoidSections.js
index dcdbb72..f8e21d6 100644
--- a/lib/parsoidSections.js
+++ b/lib/parsoidSections.js
@@ -27,13 +27,14 @@
* Gets the sections array, including HTML string and metadata for all sections
* (id, anchor, line, toclevel).
* @param {!document} doc the parsed DOM Document of the Parsoid output
+ * @param {!Logger} logger the app's bunyan logger
* @return {!sections[]} an array of section JSON elements
*/
-function getSectionsText(doc) {
+function getSectionsText(doc, logger) {
if (!hasParsoidSections(doc)) {
return parsoidSectionsUsingDivs.getSectionsText(doc);
} else {
- return parsoidSectionsUsingSectionTags.getSectionsText(doc);
+ return parsoidSectionsUsingSectionTags.getSectionsText(doc, logger);
}
}
diff --git a/lib/parsoidSectionsUsingSectionTags.js
b/lib/parsoidSectionsUsingSectionTags.js
index 76e471d..ce27178 100644
--- a/lib/parsoidSectionsUsingSectionTags.js
+++ b/lib/parsoidSectionsUsingSectionTags.js
@@ -1,8 +1,6 @@
'use strict';
const parsoidDomUtils = require('parsoid-dom-utils');
-// const sUtil = require('./util');
-// const HTTPError = sUtil.HTTPError;
const NodeType = require('./nodeType');
/**
@@ -39,7 +37,13 @@
}
}
-/* function throwIfPreviousSectionIsIncomplete(allSections, sectionObj) {
+// skip lead sections since they don't have headings;
+// no line and no anchor means incomplete section
+function shouldLogInvalidSectionWarning(sectionObj) {
+ return sectionObj.id >= 1 && !sectionObj.line && !sectionObj.anchor;
+}
+
+function validatePreviousSection(logger, allSections, sectionObj) {
if (allSections.length === 0) {
return;
}
@@ -47,30 +51,29 @@
if (!sectionObj) {
sectionObj = allSections[allSections.length - 1];
}
- // skip lead sections since they don't have headings;
- // no line and no anchor means incomplete section
- if (allSections.length > 1 && !sectionObj.line && !sectionObj.anchor) {
- throw new HTTPError({
- status: 502,
- type: 'unsupported_section',
- title: 'Section structure not supported',
+
+ if (shouldLogInvalidSectionWarning(sectionObj)) {
+ logger.warn({
+ warning: 'invalid_section',
+ title: 'Found section without expected heading',
detail: `Cannot find heading for section number ${sectionObj.id}.`
});
}
-} */
+}
/**
* Visits one DOM node. Do the stuff that needs to be done when a single DOM
node is handled.
* In this case, starts a new section object when a <section> tag is
encountered that is a
* direct descendant
+ * @param {!Logger} logger the app's bunyan logger
* @param {!DOMNode} node the node to visit
* @param {!Object[]} allSections the array containing the results
* @param {!Object} state some state to pass around
*/
-function visit(node, allSections, state) {
+function visit(logger, node, allSections, state) {
let sectionObj = allSections.length > 0 ? allSections[allSections.length -
1] : undefined;
if (node.tagName === 'SECTION') {
- // throwIfPreviousSectionIsIncomplete(allSections, sectionObj);
+ validatePreviousSection(logger, allSections, sectionObj);
sectionObj = { id: getSectionNumber(node), text: '' };
allSections.push(sectionObj);
@@ -99,16 +102,17 @@
* This started out with a traditional iterative DFS algorithm but added a
boolean to stop
* descending so we don't duplicate content since elsewhere outerHTML is used
(which contains
* content of sub-nodes).
+ * @param {!Logger} logger the app's bunyan logger
* @param {!DOMElement} rootElement the root of the DOM tree which needs to be
traversed
* @param {!Object[]} allSections holds the results
*/
-function traverseDF(rootElement, allSections) {
+function traverseDF(logger, rootElement, allSections) {
const state = {};
let nodesToVisit = [ rootElement ];
while (nodesToVisit.length > 0) {
const currentNode = nodesToVisit.shift();
- visit(currentNode, allSections, state);
+ visit(logger, currentNode, allSections, state);
if (!state.pauseDescent) {
nodesToVisit = [
...(currentNode.childNodes || []), // depth first
@@ -117,12 +121,12 @@
}
}
ensureLeadSection(allSections);
- // throwIfPreviousSectionIsIncomplete(allSections);
+ validatePreviousSection(logger, allSections);
}
-function getSectionsText(doc) {
+function getSectionsText(doc, logger) {
const allSections = [];
- traverseDF(doc.body, allSections);
+ traverseDF(logger, doc.body, allSections);
return allSections;
}
@@ -130,6 +134,7 @@
addSectionTags,
getSectionsText,
testing: {
- parseSections: traverseDF
+ parseSections: traverseDF,
+ shouldLogInvalidSectionWarning
}
};
diff --git a/test/lib/parsoid/parsoid-sections-section-elements-tests.js
b/test/lib/parsoid/parsoid-sections-section-elements-tests.js
index 77cb023..ecfb94c 100644
--- a/test/lib/parsoid/parsoid-sections-section-elements-tests.js
+++ b/test/lib/parsoid/parsoid-sections-section-elements-tests.js
@@ -1,11 +1,13 @@
+/* eslint-disable prefer-template */
+
'use strict';
const assert = require('../../utils/assert.js');
const domino = require('domino');
const parsoidSectionsUsingSectionTags =
require('../../../lib/parsoidSectionsUsingSectionTags');
+const shouldWarn =
parsoidSectionsUsingSectionTags.testing.shouldLogInvalidSectionWarning;
describe('lib:parsoid-sections (section elements)', function() {
- /* eslint-disable prefer-template */
this.timeout(20000); // eslint-disable-line no-invalid-this
@@ -119,14 +121,32 @@
assertSection0(sections, sectionInDiv);
});
- it.skip('non-lead section without heading tag should throw error', () => {
- const doc = domino.createDocument(
- '<section data-mw-section-id="0">text0</section>' +
- '<section data-mw-section-id="1">text1</section>');
- assert.throws(() => {
- parsoidSectionsUsingSectionTags.getSectionsText(doc);
- }, /unsupported_section/);
+ it('should not warn for page containing only a lead section', () => {
+ const allSections = [ { id: 0 } ];
+ assert.ok(!shouldWarn(allSections[0]));
});
- /* eslint-enable prefer-template */
+ it('should warn for non-lead section without heading tag', () => {
+ const allSections = [ { id: 0 }, { id: 1 } ];
+ assert.ok(shouldWarn(allSections[1]));
+ });
+
+ it('should not warn if id & anchor are found for all sections after the
lead section', () => {
+ const allSections = [ { id: 0 }, { id: 1, line: 'Foo', anchor: 'Foo' }
];
+ assert.ok(!shouldWarn(allSections[1]));
+ });
+
+ it('should not warn for non-lead non-editable section without heading
tag', () => {
+ const allSections = [ { id: 0 }, { id: -1 } ];
+ assert.ok(!shouldWarn(allSections[1]));
+ });
+
+ it('should not warn if a non-editable section precedes the true lead
section', () => {
+ const allSections = [ { id: -1 }, { id: 0 }, { id: 1, line: 'Foo',
anchor: 'Foo' } ];
+ assert.ok(!shouldWarn(allSections[1]));
+ });
+
+ it('should throw if sectionObj is invalid', () => {
+ assert.throws(() => { shouldWarn(undefined); }, /TypeError/);
+ });
});
--
To view, visit https://gerrit.wikimedia.org/r/398183
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2394ee5d14b7cb83f329c7f17e735a8f8d6342f9
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/services/mobileapps
Gerrit-Branch: master
Gerrit-Owner: BearND <bsitzm...@wikimedia.org>
Gerrit-Reviewer: Mholloway <mhollo...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
