MBS-9215: Add entity type to attribute editing form #471

yvanzo · 2017-02-19T18:44:49Z

Collection and series types admin editing form missed entity type.

This adds support for entering entity type without specific check.

Collection and series types admin editing form missed entity type. This adds support for entering entity type without specific check.

mwiencek · 2017-02-20T15:17:15Z

lib/MusicBrainz/Server/Form/Admin/Attributes.pm

 sub options_parent_id {
    my ($self, $model) = @_;
    return select_options_tree($self->ctx, $self->ctx->stash->{model}, accessor => 'name');
 }

+sub options_entity_type {
+    my ($self) = @_;
+    return map { $_ => $_ } sort { $a cmp $b } entities_with('collections');


Apparently this works with lists too (elsewhere we use array refs).

mwiencek · 2017-02-20T15:19:20Z

Ideally, it would validate that the parent attribute has the same entity type, but hopefully admins won't make that mistake.

mwiencek

On second thought, it'd be dangerous if the entity type of an existing collection or series type was changed, so I think we should at least validate that (that the type isn't being used).

yvanzo · 2017-02-20T17:06:28Z

I would prefer to simply forbid modifying entity type field for any existing attribute type. This could be checked in Controller::Admin::Attributes.

On a side note, there are many other ways for admins to create flawed attribute types:

change an entity type into another one different from the one of its parent/children
create a new attribute type with an entity type different from its parent’s one
change a parent id to an attribute type with a different entity type
loop with parent ids

Hopefully they can be fixed by the same admins using the same form and, more importantly, they don’t corrupt existing entities (or maybe they do but hopefully admins won't make that mistake).

mwiencek · 2017-02-20T19:06:58Z

Agreed on preventing modifying the entity type altogether, since that is most likely to cause issues, and doesn't really have a good use case. Good point about the others. I think the parent/child issues mentioned are probably not worth validating, since they wouldn't cause anything terrible if they happened.

yvanzo · 2017-02-22T15:32:24Z

Done in d7a3993

mwiencek · 2017-02-24T20:04:19Z

lib/MusicBrainz/Server/Controller/Admin/Attributes.pm

+
+        if ($old_entity_type ne $new_entity_type) {
+            $form->field('entity_type')->add_error(
+                l('Entity type \'{entity_type}\' cannot be changed; please delete and readd attribute instead.',


I think the <select> should be grayed/disabled on the editing form, and we should simply filter out entity_type from $form->edit_fields before calling update below. We're not worried about admins abusing the form, so no need to give translators a new message nobody will ever see. :)

About this, messages from admin interface should probably not be localized. Right?

Hmm, I think it's probably right for them to be localized. Even though all the admins know English, they might have a different language selected in the UI and it would be weird to make an exception here. But I'd avoid adding strings to the admin UI where not necessary, or I'd make them simple if required.

Done in 73daae6.
On a side note: I disabled the <select> element using JS because TT seems to ignore disabled attribute.

mwiencek · 2017-02-28T09:02:25Z

lib/MusicBrainz/Server/Controller/Admin/Attributes.pm

@@ -93,6 +93,8 @@ sub edit : Chained('attribute_base') Args(1) RequireAuth(account_admin) {
    my $form = $c->form( form => $form_name, init_object => $attr );

    if ($c->form_posted && $form->process( params => $c->req->params )) {
+        $form->field("entity_type")->value( $attr->entity_type );


There's a small problem: disabled fields aren't submitted by the browser, so if an error occurs in another field (try entering non-numeric characters in child order), the page will reload and the <select> will have no selection. To fix that, I think this should go above $form->process (edit: or maybe not).

I removed it in cde0df9 (where field entity_type input is enabled just before form submission) since it is unnecessary anyway, unless we want proper validation which should then take place in a separate package for attributes with entity_type field.

I think it was fine to keep it, but that's good enough for me. :)

mwiencek · 2017-02-28T09:04:34Z

root/admin/attributes/form.tt

@@ -34,6 +34,17 @@
        </p>

    [% ELSE %]
+        [%~ IF model == "CollectionType" || model == "SeriesType" ~%]
+            [% form_row_select(r, 'entity_type', add_colon(l('Entity type'))) %]
+            [%~ IF r.form.field('entity_type').value.size ~%]


Also related to errors happening in other fields: this will disable the field even if you're on the "create" form. But that's a minor issue we can probably ignore.

Thank you for having spotted this issue.
I replaced the faulty condition by a cleaner one on c.action.name in cde0df9.

There is no good reason to allow modifying entity type of CollectionType and SeriesType attributes. The only use case would be to fix careless mistake before the edited attribute gets being used. This is safer and simpler to disallow changing it altogether until finer-grained fields validation is implemented, in particular checking over related entities and parent/children attributes.

yvanzo · 2017-03-01T20:44:58Z

Failed tests are unrelated to this change.

MBS-9215: Add entity type to attribute editing form

3db8403

Collection and series types admin editing form missed entity type. This adds support for entering entity type without specific check.

mwiencek reviewed Feb 20, 2017

View reviewed changes

mwiencek approved these changes Feb 20, 2017

View reviewed changes

mwiencek requested changes Feb 20, 2017

View reviewed changes

mwiencek reviewed Feb 24, 2017

View reviewed changes

yvanzo force-pushed the mbs-9215-fix-admin-attr branch from d7a3993 to 73daae6 Compare February 27, 2017 17:11

mwiencek requested changes Feb 28, 2017

View reviewed changes

yvanzo force-pushed the mbs-9215-fix-admin-attr branch from 73daae6 to cde0df9 Compare March 1, 2017 20:29

mwiencek approved these changes Mar 1, 2017

View reviewed changes

mwiencek merged commit ca8720d into metabrainz:master Mar 1, 2017

yvanzo deleted the mbs-9215-fix-admin-attr branch April 2, 2017 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBS-9215: Add entity type to attribute editing form #471

MBS-9215: Add entity type to attribute editing form #471

yvanzo commented Feb 19, 2017

mwiencek Feb 20, 2017

mwiencek commented Feb 20, 2017

mwiencek left a comment

yvanzo commented Feb 20, 2017 •

edited

mwiencek commented Feb 20, 2017

yvanzo commented Feb 22, 2017

mwiencek Feb 24, 2017

yvanzo Feb 24, 2017

mwiencek Feb 24, 2017

yvanzo Feb 27, 2017

mwiencek Feb 28, 2017 •

edited

yvanzo Mar 1, 2017

mwiencek Mar 1, 2017

mwiencek Feb 28, 2017

yvanzo Mar 1, 2017

yvanzo commented Mar 1, 2017

MBS-9215: Add entity type to attribute editing form #471

MBS-9215: Add entity type to attribute editing form #471

Conversation

yvanzo commented Feb 19, 2017

Choose a reason for hiding this comment

mwiencek commented Feb 20, 2017

mwiencek left a comment

Choose a reason for hiding this comment

yvanzo commented Feb 20, 2017 • edited

mwiencek commented Feb 20, 2017

yvanzo commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwiencek Feb 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvanzo commented Mar 1, 2017

yvanzo commented Feb 20, 2017 •

edited

mwiencek Feb 28, 2017 •

edited