Fix #8758 - Revalidate disconnected_way and impossible_oneway errors on change

[andrewpmk]

Here is a potential fix to the issue causing disconnected road and impossible one way errors to not be reset when another road or other way is connected to them (which should cause the issue to disappear). This change simply revalidates ALL issues that are either "disconnected_way" or "impossible_oneway" whenever any change is made. This will lead to unnecessary revalidation in some cases, not sure if the performance penalty of doing this is acceptable or whether we will need to find a way to filter which of these ways get revalidated somehow.

This is my first pull request to iD, so I am not that familiar with this project and there may be a better way to do this. Sorry haven't created any tests but the existing tests pass.

Fixes #8758

[andrewpmk]

Added a test to test/spec/core/validator.js and another one to test/spec/validations/disconnected_way.js. The important one is the first one:

    it('removes validation issue when highway is no longer disconnected', function(done) {
        // Add a way which is disconnected from the rest of the map
        var n1 = iD.osmNode({ id: 'n-1', loc: [4, 4] });
        var n2 = iD.osmNode({ id: 'n-2', loc: [4, 5] });
        var w = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { 'highway': 'unclassified' } });
        context.perform(
            iD.actionAddEntity(n1),
            iD.actionAddEntity(n2),
            iD.actionAddEntity(w)
        );
        var validator = new iD.coreValidator(context);
        validator.init();
        var issues = validator.getIssues();
        expect(issues).to.have.lengthOf(0);
        validator.validate().then(function() {
            // Should produce disconnected way error
            issues = validator.getIssues();
            expect(issues).to.have.lengthOf(1);

            // Add new node with entrance node to simulate connection with rest of map
            var n3 = iD.osmNode({ id: 'n-3', loc: [4, 6], tags: { 'entrance': 'yes' } });
            var w2 = iD.osmWay({ id: 'w-2', nodes: ['n-2', 'n-3'], tags: { 'highway': 'unclassified' } });
            context.perform(
                iD.actionAddEntity(n3),
                iD.actionAddEntity(w2)
            );
            validator.validate().then(function() {
                // Should be no errors
                issues = validator.getIssues();
                expect(issues).to.have.lengthOf(0);
                done();
            }).catch(function(err) {
                done(err);
            });
        }).catch(function(err) {
            done(err);
        });

        window.setTimeout(function() {}, 20); // async - to let the promise settle in phantomjs
    });

This test adds a way that is disconnected from the rest of the map, then adds another way with entrance=yes at the end which simulates it being connected to the map (saw some other tests which do this). This test should pass with my change to revalidate disconnected_way and impossible_oneway errors, but fail without that change.

This test would be cleaner if we used an async function, but .eslintrc is currently set to ECMAScript 6 which doesn't support async functions.

[cicku]

Is this also the fix for #8900 and #8981?

[cicku]

FYI I also saw a similar warning when changing the waterway direction.

[tyrasd]

Thanks for this fix and sorry for the delay for getting this merged.

I've changed the logic slightly and extended the fix to the inverse case of the original bug: When a way becomes disconnected, the validation also should be re-triggered to warn about the newly disconnected ways.

FYI: There is currently a bug in the test runner, meaning that the test cases here don't fully work as intended, see #10773