openstreetmap / iD

🆔 The easy-to-use OpenStreetMap editor in JavaScript.
https://www.openstreetmap.org/edit?editor=id
ISC License
3.38k stars 1.21k forks source link

Warn user when uploading a boundary relation member with no role #8709

Open ZeLonewolf opened 3 years ago

ZeLonewolf commented 3 years ago

Currently, iD will allow a user to upload a boundary relation member with a blank role. Boundary relations should have the role of inner, outer, label, or admin_centre.

Steps to recreate:

  1. Click on a way that's a member of a boundary relation
  2. Delete the member's role
  3. Press upload

No warning would be shown to the user. iD should prevent the user from changing boundary relation member roles to anything other than the four values above.

1ec5 commented 3 years ago

Boundary relations should have the role of inner, outer, label, or admin_centre.

subarea is also common. There’s some disagreement about whether it’s appropriate, but it hasn’t been formally documented as being deprecated.

No warning would be shown to the user.

This is reminiscent of the existing warning (with two suggested fixes) when a multipolygon relation member is missing a role:

https://github.com/openstreetmap/iD/blob/fa5e7bca20ed38da3ae0ef87f7d3d7e7bdfd4827/modules/validations/missing_role.js#L22-L28

It would make sense to adapt this validator rule to boundary relations. This should be more straightforward than https://github.com/openstreetmap/iD/issues/8286#issuecomment-865144022, because there wouldn’t be any need to know about any other boundary relation members.

iD should prevent the user from changing boundary relation member roles to anything other than the four values above.

4695 and #8268 express a similar idea about turning the role field into a combo box with restricted values. One benefit would be the ability to translate the roles to help non-English-speakers understand relations a little better. However, restricting the values of this field would be a bit more heavy-handed than restricting the values of a normal field, since there isn’t a “raw relation editor” for a more advanced user in the middle of a tricky refactor to fall back to.

bhousel commented 3 years ago

Strong #2014 vibes from this one.

natfoot commented 2 years ago

In this case we are showing that if you edit and existing relation and there is already role errors that ID does not show those errors or warnings. Example: https://youtu.be/IaE4XDTd3l8

natfoot commented 2 years ago

This issue exists in the Multiploygon relations as well.

1ec5 commented 2 years ago

This issue exists in the Multiploygon relations as well.

Can you open a separate issue about this and include full steps to reproduce? There is a validation rule about missing roles in multipolygon relations that normally works, whereas this feature appears to be unimplemented for boundary relations:

https://github.com/openstreetmap/iD/blob/ce136d0144487bc3c8fb569910958865860559a3/modules/validations/missing_role.js#L22 https://github.com/openstreetmap/iD/blob/ce136d0144487bc3c8fb569910958865860559a3/modules/osm/relation.js#L244-L246