Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ZeLonewolf opened this issue Sep 21, 2021 · 5 comments
Open

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

ZeLonewolf opened this issue Sep 21, 2021 · 5 comments

Comments

@ZeLonewolf
Copy link

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
Copy link
Collaborator

1ec5 commented Sep 21, 2021

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:

} else if (entity.type === 'relation' && entity.isMultipolygon()) {
entity.indexedMembers().forEach(function(member) {
var way = graph.hasEntity(member.id);
if (way && isMissingRole(member)) {
issues.push(makeIssue(way, entity, member));
}
});

It would make sense to adapt this validator rule to boundary relations. This should be more straightforward than #8286 (comment), 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
Copy link
Member

bhousel commented Sep 21, 2021

Strong #2014 vibes from this one.

@natfoot
Copy link

natfoot commented Apr 28, 2022

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
Copy link

natfoot commented Apr 28, 2022

This issue exists in the Multiploygon relations as well.

@1ec5
Copy link
Collaborator

1ec5 commented Apr 28, 2022

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:

} else if (entity.type === 'relation' && entity.isMultipolygon()) {

iD/modules/osm/relation.js

Lines 244 to 246 in ce136d0

isMultipolygon: function() {
return this.tags.type === 'multipolygon';
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants