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

MBS-10432 / MBS-10433 / MBS-10437: Block adding removal edits for areas that shouldn't be removed #1240

Merged
merged 3 commits into from Oct 21, 2019

Conversation

@reosarevok
Copy link
Member

reosarevok commented Oct 17, 2019

Places use areas too, but currently we allow entering a removal
edit for an area that's only being used in places. This edit can't
be applied, since the database has a foreign key, but it shouldn't
be possible to enter in the first place.
@reosarevok reosarevok added the Bug label Oct 17, 2019
@reosarevok reosarevok requested review from mwiencek and yvanzo Oct 17, 2019
@reosarevok reosarevok added this to the Mommy Shark milestone Oct 17, 2019
lib/MusicBrainz/Server/Data/Area.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Data/Area.pm Show resolved Hide resolved
@reosarevok reosarevok force-pushed the reosarevok:MBS-10432 branch from f556351 to 4653320 Oct 17, 2019
Any area in the country_area table can't be removed anyway,
since there are constraints at the database level blocking that.
We shouldn't allow entering an edit trying to remove a country area.

This makes the release country check in can_delete redundant, so
replacing that with a call to is_release_country_area.
@reosarevok reosarevok force-pushed the reosarevok:MBS-10432 branch from 4653320 to 22d8773 Oct 17, 2019
Copy link
Contributor

yvanzo left a comment

Shouldn’t it check for existing relationship too? It is the only way to link an event to an area.

@reosarevok

This comment has been minimized.

Copy link
Member Author

reosarevok commented Oct 17, 2019

I'm happy to make that change, it just didn't seem like a bug, just a decision, unlike the others :) IMO that'd be a reasonably sensible change.

@reosarevok

This comment has been minimized.

Copy link
Member Author

reosarevok commented Oct 18, 2019

Made a change to only allow removing empty areas.

@reosarevok reosarevok changed the title MBS-10432 / MBS-10433: Block adding removal edits for areas that can't be removed MBS-10432 / MBS-10433: Block adding removal edits for areas that shouldn't be removed Oct 18, 2019
@reosarevok reosarevok changed the title MBS-10432 / MBS-10433: Block adding removal edits for areas that shouldn't be removed MBS-10432 / MBS-10433 / MBS-10437: Block adding removal edits for areas that shouldn't be removed Oct 18, 2019
@yvanzo
yvanzo approved these changes Oct 21, 2019
Copy link
Contributor

yvanzo left a comment

It would be nice to explain in latest commit’s message that release_event.country doesn’t need to be checked because country removal is already blocked with previous commit/ticket. The rest works for me! 🚀

@reosarevok reosarevok force-pushed the reosarevok:MBS-10432 branch from 80723de to a6474be Oct 21, 2019
@reosarevok

This comment has been minimized.

Copy link
Member Author

reosarevok commented Oct 21, 2019

bitmap pointed that there's no particular reason why we can't have this inside can_delete instead, since nothing else uses is_empty anyway. So I moved the check inside that (and added a bit of info to the commit message as requested!)

If an area has any relationships, it's safer not to allow removing it.
This also blocks removing it if it has open edits. It makes sure a
removal edit is not opened if the area is already being merged,
and since location editors are pretty much always going to be
autoeditors, they shouldn't have a problem approving most
other edits if needed.

Because all the previous checks for can_delete are subsets of this new
query, we can just remove them and only leave this check plus the
check for whether the area is a release country (where we still want to
block removing it even if it is otherwise empty!). Since the release
country check will apply to all areas used on releases, there is no need
to check the release_country table in this query.
@reosarevok reosarevok force-pushed the reosarevok:MBS-10432 branch from a6474be to 7707249 Oct 21, 2019
@yvanzo
yvanzo approved these changes Oct 21, 2019
Copy link
Contributor

yvanzo left a comment

:shipit:

@reosarevok reosarevok merged commit 8a8111d into metabrainz:master Oct 21, 2019
4 checks passed
4 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: js Your tests passed on CircleCI!
Details
ci/circleci: perl-and-pgtap Your tests passed on CircleCI!
Details
ci/circleci: selenium Your tests passed on CircleCI!
Details
@reosarevok reosarevok deleted the reosarevok:MBS-10432 branch Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.