MBS-10499: Alert for entity in danger of removal once add edit passes

[reosarevok]

Implement MBS-10499

Problem

Sometimes users (especially new ones) add an entity as a votable edit (by checking the relevant checkbox) and are then not notified that the entity is in risk of removal, since their add edit means there's a pending edit keeping the entity around. Unless they have added more data, though, the entity will be removed soon after the one pending edit passes.

Solution

This checks if the creation edit is the only thing keeping the entity from being otherwise eligible for cleanup. If that's the case, it then notifies the user with the same banner as if there was no data there at all.

Notice that is_empty no longer checks edits directly. The RemoveEmpty autoremoval script actively checks for edits anyway, so there should be no risk this change would cause the wrong things to be removed.

Testing

Manually tested (by adding some new edits with votable additions). I'm also adding a test to see if the banners show as expected for artist at least, since AFAICT we were not testing this at all.

Dependencies

On top of #2638

[mwiencek]

Hmm. Do we need separate banners for these two cases? It seems like we could just remove all the edit-checking code from is_empty and check here if edits_pending == 0 || (edits_pending == 1 && creation_edit->is_open), then just show a generic message about the entity being in danger of removal within the next few days.

(But we should also change the RemoveEmpty script so that that's actually true; it appears to remove things that are empty after only 1 day.)

[reosarevok]

Yes, I think it's always been 1 day, "the next few days" was always in case it failed to run or something 😄

We'd still need to check if it's empty, right? It might be other edits adding more data have been made as autoedits. But that could check only for data and not edits, yes.

[mwiencek]

Right, we'd still check if it's empty in any case. One banner for both cases + changing the RemoveEmpty interval to 3 days or something makes sense to me. Do you see any issue with combining them?

[reosarevok]

The only issue with moving the RemoveEmpty interval to 3 days is that it adds more risk of someone actually using the stuff before it gets deleted (say, if an editor empties an incorrect label in hopes of it getting removed). Not sure how common that is, though.

[reosarevok]

Another issue is for stuff like vandalism you'd probably want to get rid ASAP once empty, but I guess for that case we could add a manual way of doing it :)

[reosarevok]

So if we did this, what would remain in is_empty? Something like this only?

    my $used_in_relationship = used_in_relationship($self->c, artist => 'artist_row.id');
    return $self->sql->select_single_value(<<~"SQL", $artist_id);
        SELECT TRUE
        FROM artist artist_row
        WHERE id = ?
        AND NOT (
          EXISTS (
            SELECT TRUE FROM artist_credit_name
            WHERE artist = artist_row.id
            LIMIT 1
          ) OR
          $used_in_relationship
        )
        SQL

I guess the RemoveEmpty script does its own separate check for emptiness so that wouldn't necessarily be a problem to remove from here.

Also, I'm assuming we'd have a separate ticket for the 1 day -> 3 days RemoveEmpty change? :)

[reosarevok]

I tried to implement this more or less as talked, see if it looks good now :)

[yvanzo]

Successfully tested with/without votable checkbox/related entities for a new Place.

[mwiencek]

Tested this and it seems to work well. Though if we can avoid some unneeded queries in Role::Cleanup, let's do that.

[mwiencek]

LGTM, but I didn't test every single entity nor check every add_edit_type value added to entities.json. :)

[reosarevok]

@brainzbot, retest this please