Keep right QA #5201

thomas-hervey · 2018-08-03T20:50:18Z

After the recent introduction of notes in iD, I've been discussing with @kamicut and @bhousel about the future of QA tools. A new QA section under "Map Data" could be used to display QA issues, such as KeepRight, a data consistency check that automatically identifies issues.

bhousel · 2018-08-11T02:37:32Z

So excited to try this out!! @thomas-hervey I know you've been working on it the past few days, but it looks like tests are passing again. Let me know if it's ready!

thomas-hervey · 2018-08-11T03:59:20Z

@bhousel, go ahead and test it out. I will be pushing small updates, but at this point it's ready for checking and cleaning.

magol · 2018-08-26T08:20:59Z

@thomas-hervey
I am looking forward to this being ready. How is the status of your job? Is it possible to test it already?

thomas-hervey · 2018-08-27T21:37:29Z

@magol thanks for your interest. I have been on the road so my work has been intermittent over the last weeks. I am going to work on it a bit more today and I hope to get feedback from @bhousel. However, feel free to checkout the branch and play around with it.

tordans · 2018-08-31T06:18:10Z

Hi @thomas-hervey can you give me a hand testing this? I cannot find the new feature in my dev editor.
What I did:

follow https://github.com/openstreetmap/iD#installation
checkout keep-right_QA
run the rpm commands from 1 again
open http://localhost:8080/#background=Berlin-2018&disable_features=boundaries&map=17.00/52.51669/13.39015

How would I see the new feature? I dont find it in the panels (eg next to the nodes?) or on the map itself.

Thanks

thomas-hervey · 2018-08-31T16:02:33Z

@tordans, I reproduced your steps and everything should be working. To turn on the layer, look under Quality Assurance at the bottom of the Map Data menu (hotkey F). There is a toggle for KeepRight.

At this point, there are several outstanding tasks including tweaking the post service, tile optimization, more complex translations, custom settings (in the works), and more. Let me know if you still have issues.

kymckay · 2018-08-31T16:17:00Z

I see there used to be an issue with the error description parsing on error type 411 (http error) due to the URL being collected together with the surrounding tag data (href="URL">URL</a>)). Which would result in:

The URL (<a target="_blank" href="\href=URL>URL</a>)\">{var1}</a>) cannot be opened (HTTP status code 503)

I was looking into this until I realised some of the newer commits resolved the issue, but it got me thinking about the way you're parsing the descriptions in parseErrorDescriptions. Is there any reason you opted not to just extract the details you're looking for by capturing them as regex groups? It would save all the hassle of iterating over the split strings as well as prevent having to add special cases for the descriptions that don't have details split neatly between space characters (like type 220).

The strings you have in errorSchema.json are effectively patterns of a different syntax, in a way you're reinventing the wheel there 😄

Edit: I'd be happy to submit a PR to this branch with changes along those lines if desirable.

thomas-hervey · 2018-08-31T16:42:35Z

@SilentSpike thanks for your input. Hmm I see what you're saying. As far as that particular error, atm I opted to scratch parsing it and remove the actual URL for safety reasons. No I didn't use string.split() for a particular reason except for simplicity, and regex would be used for identifying IDs. But yes at this point something more consistent like regex groups could help. A PR would be great. I'm only able to spend part time working on this atm.

tordans · 2018-09-01T05:23:11Z

Thanks @thomas-hervey for helping with #5201 (comment), its working now; issue on my side.

I really like the idea of this PR. Therefore I collected some feedback below. Thanks for this feature!

I collected my first impression

My goal was to collect, not to find solutions :).

a. I experienced the zoom problem from #5169. I needed to zoom out to find an QA-issue which caused the page to become very slow due to a lot of data; afterwards zooming in was tedious.

Update: I found that disabling the OSM-layer helps, but leads to (j) and (k). Its also very manual.

b. I found https://www.keepright.at/report_map.php?schema=100&error=116761974 / http://localhost:8080/#background=Berlin-2018&disable_features=buildings,landuse,boundaries&map=21.00/52.51477/13.38943 but it took me quite a while to understand what the issue is related to, since I did not understand the "way 35557143" (left panel) as the part that is most relevant for this. This element connects the OSM layer with the QA layer and allows me to "select the OSM-element in question". How could that be made clearer?

b. I then tried out to understand the issue and how to fix it. I noticed, that I cannot click the node below the ⚡️-icon. Thats a problem IMO. The only way I could access the pin below the ⚡️was by disabling the QA layer again, that feels like a bad solution. (b) does not help here, since (b) selects the way (for a way-issue), but I wanted to check the node of the way since I assumed an issue there. The node was right below the ⚡️.

The same came up with the second issue that I looked at, an broken URL. The pin is below the ⚡️. In this case (b) helped to select the element in question.

c. For me this issue looks like a false positive. But what do I do with that?

how can I check the rules that created the issue to better understand if its a false positive?
should I just say "ignore"? But does "ignore" actualy mean? Will it hide the issue from everyone immediately and forever or just from me?

d. Is there a validation on the comment field? I feel like it should validate to at least 2 word for ignore cases, right? I dont want to test it since I dont know what will happen and the data are live, right?

d2. Keepright actually has thee options "solve, ignore for me, false positive for everyone". Which one are used here / how do they translate to the buttons?

e. I image when I could actually fix the issue, I have the same problem as with #5170, I would like to manually sync my commit message with the get-right-issue. This is the same for HOT Task manager btw, I type my change twice, once in the task manager, once in the commit message. Could the QA layer maybe pre-fill the commit-message with a hash-tag like "#keepright #keepright-issue-123" so there is some link?

f. How do I get from one issue to the next one? Lets say I solved this one, how do I find the next one? The only ways seems to be to zoom out again (see a). But maybe on the post-comment-screen there is a list of the next issues? I will check once I find one that I can solve :).
Update: See (g), since "comment" does not show a post-comment-page ("issue solved, here is the next issue-nearby-list"), I really think discovering the next issue is still a problem.

g. I am looking for a way to share a specific keepright-issue on iD editor. I can share the Keepright-URL, which is provided in iD, but what I was hoping/looking for is: Once I select an KR-issue, the iD URL changes to reflect this issue.

h. I solved my second issue, but now I am very confused about how the comment-field works.

once I click comment, nothing happens(?)
however, apparently the comment is saved, because I find it in keepright (the original UI)
however, it does not behave like a comment field but a "note" field that I can edit at any time? Or is it even one note field for all users, so anyone can edit it?
This is very unclear to me ATM.

i. To stress (h), the color schema for the ⚡️ is problematic. I did not see the second issue which was a broken URL on the map since it is the same color as buildings, but will be placed on buildings very often. Green is also a hard to see color in nature.

j. When searching for the next ⚡️I find myself disabling the "OSM Data" checkbox because of (a) and (h). However, I also need to disable the QA-Layer because of (b), I scroll around in the panel a lot because OSM is at the top, QA at the bottom.

k. There is a weird behavior that happens with (j): I have OSM data disabled, click on a ⚡️ and inside the ⚡️ on the relation element (see b). Now the UI enables OSM data again (which is very smart, because I need those to see the issue, BUT it does not select the given node. Instead it shows the "choose a tag" UI in the left panel. I found that I need to manually de- and re-activate the OSM data layer, only then does (b) really select the element.

Update: Maybe the last part is not right, I could not solve the issue in every case by re-enabling the layer. Also soft reload of the page did not help. The way of https://www.keepright.at/report_map.php?schema=100&error=102733535 is select (when clicking (b)) in the editor UI (glow) but not visible in the left panel.

kymckay · 2018-09-01T16:57:01Z

Minor bug I've found while testing. This error (type 70) seems to show up as the wrong error type (I've had 41 and 50) in the iD layer.

thomas-hervey · 2018-09-05T16:47:45Z

@tordans, thanks for your feedback. Looking into your list of errors now. Hopefully some are handled by @SilentSpike's refactoring ^.

kymckay · 2018-09-05T17:08:06Z

An update on my previous comment: It should be fixed in my PR (I found that what the wiki lists as error 70 is actually error 74 when inspecting the GeoJson).

kymckay · 2018-09-06T08:15:01Z

modules/util/keepRight/errorSchema.json

@@ -217,7 +217,7 @@
            },
            "_220": {
                "title": "misspelled tags",
-                "description": "This (node|way|relation) is tagged '([\\w:]+)=(.+)' where &quot;(\\2|\\3)&quot; looks like &quot;([\\w\\s]+)&quot;",
+                "description": "This (node|way|relation) is tagged '([\\w]+)(:([\\w]+))?=(.+)' where &quot;(\\2|\\3|\\4|\\5)&quot; looks like &quot;([\\w\\s]+)&quot;",


Hmm, this is an interesting case. I have a feeling this won't work for all cases because the pattern will now only match keys with 0 or 1 namespace (which doesn't handle the likes of turn:lanes:backward).

I see that the bit in quotes can be any substring of the key though (I wonder if that also applied to the value). It might be easiest to revert the key group and then just use a .+ in the group within the quotes.

matkoniecz · 2018-11-03T11:43:12Z

From what I see at https://github.com/keepright/keepright this tools seems to be not maintained anymore

last commit in February 2017 https://github.com/keepright/keepright/commits/master
unreviewed PR - Use full dates in rss export keepright/keepright#91
known false positives waiting for fix (for example Please disable or fix waterway crossing highway warnings, many are in contradiction with wiki and established practice keepright/keepright#60 )

I am not sure whatever exposing iD users to this tool is a good idea.

Maybe Osmose would be a better idea?

bhousel · 2018-11-03T13:27:57Z

From what I see at https://github.com/keepright/keepright this tools seems to be not maintained anymore
I am not sure whatever exposing iD users to this tool is a good idea.

It's not maintained much, but it does what it's supposed to do, and the server is up. I have a feeling that if iD links to it, people will take more of an interest in maintaining and keeping the server going.

Maybe Osmose would be a better idea?

Yeah, I'd like to do that one too. Also ImproveOSM, and every other Q/A thing that exists! 👍

matkoniecz · 2018-11-03T13:32:33Z

I have a feeling that if iD links to it, people will take more of an interest in maintaining and keeping the server going.

I have not considered this.

… buttons

Also implement parsing error 231 - layer conflict

The user details and auth are not needed because KeepRight accepts only anonymous updating

There are a lot of these

kymckay · 2019-01-04T16:32:51Z

data/keepRight.json

+    "73": {
+      "title": "",
+      "severity": "error",
+      "description": "This way has a tracktype tag but no highway tag"


Example of where this error requires regex:

https://www.keepright.at/report_map.php?schema=86&error=93785774

Thanks good catch @SilentSpike !

from the source https://github.com/keepright/keepright/blob/dc7a8ada2c87f7cf68d9a1a546cebb53a1f0c281/checks/0070_missing_tags.php#L97-L108

Oh nice, I hadn't even realised source was available for keepright 😄

(closes #5213)

(re: #5201 (comment) )

This makes it easier to select the KeepRight issue and avoid selecting the OSM geometry underneath them

Makes it easier to select the point/vertex that the error is about.

bhousel · 2019-01-05T03:43:30Z

I just merged this! 🎉

Thanks @thomas-hervey for working on this, and I'm looking forward to more integration with other Q/A tools. It will be great to see mappers using this to improve OpenStreetMap!

Thanks also @tordans and @SilentSpike for the detailed feedback on this thread. I've spent the past few weeks improving the user experience and was able to incorporate a lot of your feedback into the latest code. There are also some other good ideas in there that I'll open as follow up issues.

For the curious, you can try it out now in http://preview.ideditor.com/master by enabling the layer on the Map data pane.

mvl22 · 2019-01-07T19:34:34Z

Great news! - thanks :)

Looking forward to seeing this in live.

Just tried this in an edit - makes identifying connectivity problems massively quicker.

Bibi56 · 2021-05-12T22:25:47Z

From what I see at https://github.com/keepright/keepright this tools seems to be not maintained anymore
(...)
I am not sure whatever exposing iD users to this tool is a good idea.

Maybe Osmose would be a better idea?

By selecting Osmose and KeepRight in iD, I got false positive from KeepRight (name and name:XX).
So Osmose is definitively a better option: here KeepRight lower the quality of the database. A QA tool should do the opposite^^.

jjiglesiasg · 2021-05-13T17:31:27Z

In my experience, both are very useful, why to remove one of them? From: Bibi56 ***@***.*** Sent: Wednesday, May 12, 2021 6:26 PM To: openstreetmap/iD ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [openstreetmap/iD] Keep right QA (#5201) From what I see at https://github.com/keepright/keepright this tools seems to be not maintained anymore (...) I am not sure whatever exposing iD users to this tool is a good idea. Maybe Osmose would be a better idea? By selecting Osmose and KeepRight in iD, I got false positive from KeepRight (name and name:XX <https://github.com/keepright/keepright#106> ). So Osmose is definitively a better option: here KeepRight lower the quality of the database. A QA tool should do the opposite^^. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5201 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFOKQKRB7FI2RTF32K64JR3TNL6ADANCNFSM4FN2MPFA> . <https://github.com/notifications/beacon/AFOKQKVPWDKA75BBDSW2EPLTNL6ADA5CNFSM4FN2MPFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGIJW3FY.gif> <http://www.avg.com/email-signature?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=emailclient> Virus-free. <http://www.avg.com/email-signature?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=emailclient> www.avg.com

thomas-hervey requested a review from bhousel August 3, 2018 20:50

kymckay mentioned this pull request Aug 31, 2018

[KeepRight] Use regex group capture to extract error details #5275

Merged

thomas-hervey self-assigned this Sep 5, 2018

thomas-hervey added the new-feature A new feature for iD label Sep 5, 2018

kymckay reviewed Sep 6, 2018

View reviewed changes

bhousel mentioned this pull request Dec 16, 2018

Holes in ways #5607

Closed

thomas-hervey added 9 commits December 19, 2018 13:12

added simple keepRight button under photoItems

b3cffd7

WIP: added schemas for errors and warnings

b969655

displaying keep right (currently as notes)

75cff00

added basics to sidebar

31f35b0

WIP: identifying variables in errors

1827837

WIP: menu with sub-layer buttons; TODO: bug fixes, ui updates, wiring…

3e7146c

… buttons

added back supported

6dbea57

small UI change to header

cbcdfc0

added link for object in header

805dd39

bhousel added 11 commits January 2, 2019 13:39

Relax some more regex for tag parsing, (to support 'key=*')

9557c02

Also implement parsing error 231 - layer conflict

Fix comment box style, remove unused vars for eslint

4494c3a

KeepRight only allows one anonymous comment per issue - simplify

ee92758

Remove user details and auth and adjust button texts for commenting

69ce5ee

The user details and auth are not needed because KeepRight accepts only anonymous updating

Update or remove the error after POSTing changes to server

9e6497b

Remove redundant CSS

0256ad7

Fix error reporting urls and make sure sidebar not showing stale data

46ebce2

Reduce number of marker colors, increase contrast, tweak language

35f8078

Reintroduce some error localization, fix some nagging parse issues

d0343c9

Give missing maxspeed its own marker color style

10ece54

There are a lot of these

Style missing tracktype like missing maxspeed

67403a2

kymckay reviewed Jan 4, 2019

View reviewed changes

bhousel added 4 commits January 4, 2019 15:48

Use touch targets for notes, fix a few bugs with note dragging

cd92039

(closes #5213)

Convert KeepRight error 73 to regex

0582faf

(re: #5201 (comment) )

Switch KeepRight markers to use touch targets

fadd8e2

This makes it easier to select the KeepRight issue and avoid selecting the OSM geometry underneath them

Adjust distance from KeepRight marker and underlying geometry

48cc06f

Makes it easier to select the point/vertex that the error is about.

bhousel merged commit 48cc06f into master Jan 5, 2019

bhousel deleted the keep-right_QA branch January 5, 2019 03:43

This was referenced Jan 5, 2019

Visually differentiate roads speed limits #5662

Open

KeepRight interface followup issues #5679

Open

Add Keep Right integration as new layer for QA data fixing #3452

Closed

Add Osmose Q/A layer #5682

Closed

Add ImproveOSM Q/A layer #5683

Closed

1ec5 mentioned this pull request Jan 19, 2019

Highlight invalid field values #2836

Open

quincylvania mentioned this pull request Jan 23, 2019

Update to iD v2.13.0 openstreetmap/openstreetmap-website#2122

Merged

1ec5 mentioned this pull request Dec 4, 2021

Add OSM Inspector issues as a data layer #8838

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep right QA #5201

Keep right QA #5201

thomas-hervey commented Aug 3, 2018 •

edited

Loading

bhousel commented Aug 11, 2018

thomas-hervey commented Aug 11, 2018

magol commented Aug 26, 2018

thomas-hervey commented Aug 27, 2018

tordans commented Aug 31, 2018

thomas-hervey commented Aug 31, 2018 •

edited

Loading

kymckay commented Aug 31, 2018 •

edited

Loading

thomas-hervey commented Aug 31, 2018 •

edited

Loading

tordans commented Sep 1, 2018 •

edited

Loading

kymckay commented Sep 1, 2018

thomas-hervey commented Sep 5, 2018

kymckay commented Sep 5, 2018

kymckay Sep 6, 2018

matkoniecz commented Nov 3, 2018

bhousel commented Nov 3, 2018

matkoniecz commented Nov 3, 2018

kymckay Jan 4, 2019

bhousel Jan 4, 2019

kymckay Jan 4, 2019

bhousel commented Jan 5, 2019

mvl22 commented Jan 7, 2019 •

edited

Loading

Bibi56 commented May 12, 2021

jjiglesiasg commented May 13, 2021 via email

Keep right QA #5201

Keep right QA #5201

Conversation

thomas-hervey commented Aug 3, 2018 • edited Loading

bhousel commented Aug 11, 2018

thomas-hervey commented Aug 11, 2018

magol commented Aug 26, 2018

thomas-hervey commented Aug 27, 2018

tordans commented Aug 31, 2018

thomas-hervey commented Aug 31, 2018 • edited Loading

kymckay commented Aug 31, 2018 • edited Loading

thomas-hervey commented Aug 31, 2018 • edited Loading

tordans commented Sep 1, 2018 • edited Loading

I collected my first impression

kymckay commented Sep 1, 2018

thomas-hervey commented Sep 5, 2018

kymckay commented Sep 5, 2018

kymckay Sep 6, 2018

Choose a reason for hiding this comment

matkoniecz commented Nov 3, 2018

bhousel commented Nov 3, 2018

matkoniecz commented Nov 3, 2018

kymckay Jan 4, 2019

Choose a reason for hiding this comment

bhousel Jan 4, 2019

Choose a reason for hiding this comment

kymckay Jan 4, 2019

Choose a reason for hiding this comment

bhousel commented Jan 5, 2019

I just merged this! 🎉

mvl22 commented Jan 7, 2019 • edited Loading

Bibi56 commented May 12, 2021

jjiglesiasg commented May 13, 2021 via email

thomas-hervey commented Aug 3, 2018 •

edited

Loading

thomas-hervey commented Aug 31, 2018 •

edited

Loading

kymckay commented Aug 31, 2018 •

edited

Loading

thomas-hervey commented Aug 31, 2018 •

edited

Loading

tordans commented Sep 1, 2018 •

edited

Loading

mvl22 commented Jan 7, 2019 •

edited

Loading