MBS-11178: Make attribute lists visible to all users #2480

reosarevok · 2022-04-08T13:37:56Z

Implement MBS-11178

Problem

There's currently no way for users to see any info about the multitude of things we call "attributes" all in one place. That includes entity types, languages, scripts, but also stuff like gender. Many have their descriptions shown in bubbles around different editing menus, some not even that.

Solution

This takes the existing lists under /admin/attributes, hides a bunch of things like edit links that should ideally be mostly relevant to admins only, and makes them available to all users as read only tables.

Additionally, it turns the index page for the attribute types from a list of Perl model names to a list of actual translatable human words.

Testing

Manually, visiting all attribute pages while logged out, and a bunch while logged in with the right privs to make sure the extra data is shown.

mwiencek · 2023-11-15T15:16:58Z

hides a bunch of things like rowids and edit links that should ideally be mostly relevant to admins only (although it's possible that rowids are actually needed for some seeding at the moment, that should be replaced with MBIDs...)

There was someone in #musicbrainz recently who was writing a SQL query and asking if we documented the type IDs somewhere. I think we should show them, because they are useful for writing SQL queries at least (and for form submissions, as long as we use them there). Internally, these IDs are stable and there's no point joining another table just so you can use an MBID.

mwiencek

Tested by loading all the pages locally both as a normal user and as a relationship editor. Seemed to work fine.

root/static/scripts/common/utility/attributeModelName.js

yvanzo

I’m a bit puzzled to be honest. Trying to address three quite different issues in the same pull request makes it certainly more difficult to consider each issue properly.

Tested locally with sample data and it mostly worked as described in the pull request. Adding a new area type worked but triggered an ISE on every attribute page. Sorry, I don’t remember which values and didn’t save the page content. The server logs still have:

[warning] GET http://localhost:5000/attributes caused a warning: Use of uninitialized value $length in numeric lt (<) at lib/MusicBrainz/Server/Renderer.pm line 41.
[error] Caught exception in MusicBrainz::Server::View::Node->process "malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "(end of string)") at lib/MusicBrainz/Server/Renderer.pm line 47."

yvanzo · 2023-11-28T17:59:15Z

lib/MusicBrainz/Server/Controller/Attributes.pm

-package MusicBrainz::Server::Controller::Admin::Attributes;
+package MusicBrainz::Server::Controller::Attributes;


So far the term attribute was publicly used in relationship attribute and work attribute only.

For example C major is a work attribute of type Key and is stored in the table work_attribute, while _Song is just a work type among other work properties and is stored in work_type.

The term (entity) attribute was used in admin forms instead of property in the user docs.

So do you feel we should rename all these to Properties? I wouldn't mind, if it seems more clear. It seems as arbitrary a name as Attributes but at least we're not using it for something else already I guess, and this is basically a bunch of misc things, so...

yvanzo · 2023-11-28T18:17:21Z

lib/MusicBrainz/Server/Controller/Attributes.pm

-sub create : Chained('attribute_base') RequireAuth(account_admin) SecureForm {
+sub create : Chained('attribute_base') RequireAuth(relationship_editor) SecureForm {


While it is true that it doesn’t have anything to do with accounts, modifying “attributes” (or whatever their names) is not as safe as modifying relationship types at the moment: It doesn’t create edits, there are little (if any) database constraints, there is little (if any) form validation.

So it currently mostly has to do with not breaking the website at a large scale.

Editing those should probably rather require dedicated privilege flags, so should editing genre and editing instrument, to follow the existing model of dedicated privileges for editing area.

Editing those should probably rather require dedicated privilege flags

It feels these are conceptually very close to relationships (basically it's "schema editor") but I see the point about safety measures. I'm fine with having a separate option if you prefer that, shouldn't be hard to implement a new privs flag (I understand those do not require schema changes anyway) - no idea what we'd call it though! In any case, I really would prefer not to leave this for account_admin since there's no particular reason we would expect those to deal with schema.

reosarevok · 2023-11-29T06:39:47Z

I’m a bit puzzled to be honest. Trying to address three quite different issues in the same pull request makes it certainly more difficult to consider each issue properly.

These seem very closely related to me (basically it was "what needs fixing in these pages" :) ) but since two of the issues are limited to one commit each, we could move them to separate PRs depending on this one if you prefer.

Adding a new area type worked but triggered an ISE on every attribute page.

That is probably an existing bug of some sort, since I barely touched edit forms here, but I'll see if I can reproduce and fix it!
Edit: a quick test did not manage to reproduce it. Can you maybe look at the data in the local DB to figure out what you entered? 😅 I already have a different PR fixing possible crashes in these (#2776) but I suspect yours is a different kind.

reosarevok · 2023-12-04T19:47:17Z

Moved stuff outside MBS-11178 to subsequent PRs #3115 and #3116 to make this easier to review.

yvanzo · 2023-12-05T01:17:48Z

I just added the commit e76ec04 to the pull request #3091 so that we can merge it without losing the current translations that will likely be reused for resolving MBS-11178. If merged this commit will just have to be reverted here.

The attributes are rendered in Attribute, these are only a few extra headers/columns for some types.

This removes the info that is mostly useless for people without editing rights (such as edit links) but displays everything else. Row ids are not removed since documenting these on the site is apparently helpful for people putting together SQL queries (this has been requested). Since this is visible to users, translations are brought back.

This seems a lot more consistent than having them in two places.

Since this is no longer admin-only, there's no reason all of this should live inside /admin folders. Use l_admin for admin-facing strings only.

This reduces code reuse to some degree. We could make further changes to also generalize part of the tables for each model, but I'm not sure it's worth the effort. Additionally, this shows a proper translatable title for each attribute type, since we'll display them to users now. Only Language and Script were translated before.

This splits the list in two, one for entity types and one for everything else. It also sorts them by the (translated) names of the attribute types, rather than by the static model name.

This is not the index for an attribute, it's the list of all attributes. Consistent with RelationshipTypesList for reltypes.

reosarevok added the New feature Non urgent new stuff label Apr 8, 2022

reosarevok changed the title ~~MBS-11178: Make attribute lists visible to all users~~ MBS-11178 / MBS-12217: Make attribute lists visible to all users, including alias types Apr 8, 2022

reosarevok force-pushed the MBS-11178 branch 2 times, most recently from 2666934 to d13a885 Compare April 11, 2022 10:02

reosarevok force-pushed the MBS-11178 branch from d13a885 to 52c7125 Compare January 16, 2023 12:15

reosarevok added the Documentation PRs that mostly create or update documentation label Jan 24, 2023

reosarevok mentioned this pull request May 5, 2023

MBS-13060: Display attribute MBIDs on attribute tables #2930

Merged

reosarevok force-pushed the MBS-11178 branch 4 times, most recently from 47d63df to cee7cdc Compare May 30, 2023 14:41

reosarevok force-pushed the MBS-11178 branch from cee7cdc to 0e6871f Compare November 15, 2023 14:52

reosarevok force-pushed the MBS-11178 branch 2 times, most recently from 9da3fb6 to 854f4c8 Compare November 15, 2023 15:48

mwiencek approved these changes Nov 27, 2023

View reviewed changes

root/static/scripts/common/utility/attributeModelName.js Show resolved Hide resolved

reosarevok mentioned this pull request Nov 27, 2023

MBS-13117: Prevent localizing admin-only messages #3091

Merged

6 tasks

yvanzo reviewed Nov 28, 2023

View reviewed changes

reosarevok force-pushed the MBS-11178 branch from 6f69b99 to 852c6d3 Compare December 4, 2023 19:13

reosarevok mentioned this pull request Dec 4, 2023

MBS-12217: Add admin interface for editing alias types #3115

Draft

reosarevok force-pushed the MBS-11178 branch from 852c6d3 to 13bbe19 Compare December 4, 2023 19:45

reosarevok changed the title ~~MBS-11178 / MBS-12217: Make attribute lists visible to all users, including alias types~~ MBS-11178: Make attribute lists visible to all users Dec 4, 2023

reosarevok force-pushed the MBS-11178 branch from 13bbe19 to 194d16b Compare December 5, 2023 15:05

reosarevok force-pushed the MBS-11178 branch from 194d16b to 204ed2b Compare December 14, 2023 10:32

reosarevok force-pushed the MBS-11178 branch from 204ed2b to 20e45a3 Compare February 20, 2024 11:01

reosarevok force-pushed the MBS-11178 branch 2 times, most recently from d94bd39 to ba97c65 Compare June 24, 2024 14:55

reosarevok added 7 commits September 18, 2024 15:44

Clarify function names

13b9765

The attributes are rendered in Attribute, these are only a few extra headers/columns for some types.

Move generic attributes form to the same folder as others

f1cf9dd

This seems a lot more consistent than having them in two places.

Move attributes code out of admin folders

2b7e000

Since this is no longer admin-only, there's no reason all of this should live inside /admin folders. Use l_admin for admin-facing strings only.

Make list of attribute types more usable

610a7d8

This splits the list in two, one for entity types and one for everything else. It also sorts them by the (translated) names of the attribute types, rather than by the static model name.

Rename attributes/Index to AttributesList

28105a6

This is not the index for an attribute, it's the list of all attributes. Consistent with RelationshipTypesList for reltypes.

reosarevok force-pushed the MBS-11178 branch from ba97c65 to 28105a6 Compare September 18, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBS-11178: Make attribute lists visible to all users #2480

MBS-11178: Make attribute lists visible to all users #2480

reosarevok commented Apr 8, 2022 •

edited

Loading

mwiencek commented Nov 15, 2023

mwiencek left a comment

yvanzo left a comment

yvanzo Nov 28, 2023 •

edited

Loading

reosarevok Nov 29, 2023 •

edited

Loading

yvanzo Nov 28, 2023

reosarevok Nov 29, 2023 •

edited

Loading

reosarevok commented Nov 29, 2023 •

edited

Loading

reosarevok commented Dec 4, 2023

yvanzo commented Dec 5, 2023

		package MusicBrainz::Server::Controller::Admin::Attributes;
		package MusicBrainz::Server::Controller::Attributes;

		sub create : Chained('attribute_base') RequireAuth(account_admin) SecureForm {
		sub create : Chained('attribute_base') RequireAuth(relationship_editor) SecureForm {

MBS-11178: Make attribute lists visible to all users #2480

Are you sure you want to change the base?

MBS-11178: Make attribute lists visible to all users #2480

Conversation

reosarevok commented Apr 8, 2022 • edited Loading

Implement MBS-11178

Problem

Solution

Testing

mwiencek commented Nov 15, 2023

mwiencek left a comment

Choose a reason for hiding this comment

yvanzo left a comment

Choose a reason for hiding this comment

yvanzo Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

reosarevok Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

yvanzo Nov 28, 2023

Choose a reason for hiding this comment

reosarevok Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

reosarevok commented Nov 29, 2023 • edited Loading

reosarevok commented Dec 4, 2023

yvanzo commented Dec 5, 2023

reosarevok commented Apr 8, 2022 •

edited

Loading

yvanzo Nov 28, 2023 •

edited

Loading

reosarevok Nov 29, 2023 •

edited

Loading

reosarevok Nov 29, 2023 •

edited

Loading

reosarevok commented Nov 29, 2023 •

edited

Loading