Custom Attributes

[mwerner]

Hey team,

I got a significant portion of custom attributes (#92) written, but wanted to check in and see if this is in the direction you had in mind before continuing to finish the work and write specs. Let me know if this is something you could use. Thanks

An AttributeCategory has many AttributeFields. When one of the fields is provided a value, it saves the entity as a polymorphic association on the attributes table. The bulk of the remaining work is improving the UI to actually create attribute categories, since they're a little different than your existing content style models. There will also need to be a good amount of work around providing a correct value since content.send(attribute) wouldn't work with this pattern.

Pardon a few spots where there are things like literals in the conditional, this isn't a finished PR, just a PoC.

[drusepth]

Howdy! Thank you for this.

I tried to get it up and running locally to check out how things looked and functioned, but couldn't get it running properly (couldn't create content, and edit forms after creating content on the console were blank).

That said, this sounds like just what we're looking for to finish #92. I'm not sure I understand the relation between AttributeCategories and AttributeFields, though. Do content (characters, locations, items, and universes) have many AttributeCategories (and therefore many AttributeFields through AttributeCategories), or do they merely have many AttributeFields (and, if the former, why not the latter)?

As for conforming to existing paradigms, I think we could (and should) do away with just doing content.send(attribute) everywhere. What do you think about defining a content.get(attribute) method that can handle existing fields while also grabbing custom fields from the same interface?

Thanks again for doing this! This is a hugely requested feature, and I'd love to see it materialize in a nice way. :)

[mwerner]

Thanks for checking it out. I agree, using the existing forms right now is pretty broken. I'll sort that out and when I get back to this.

I started off with a user just having many attribute fields, but I soon hit a problem of "where does the field go?" when displaying it on a resource. That led me to groupings of attributes, which i called categories, because this ends up almost directly mapping attribute_categories:

# item.rb
def self.attribute_categories
  {
    overview: {
      icon: 'info',
      attributes: %w(name item_type description universe_id)
    },
    looks: {
      icon: 'redeem',
      attributes: %w(materials weight)
    }
  }
end

overview and looks are considered the AttributeCategory and name, item_type, description, universe_id, materials, and weight are all AttributeField, distributed to the corresponding attribute categories. The attribute categories end up being the tabs across the top of a resource's card. Hopefully when this is done, it could replace most of that hard-coded attribute_categories littered through the models.

There are some fields on a resource that I'd want to make sure are on there regardless of how they configure the attribute fields. Those "system fields" will probably be mostly relationships and core stuff like description. I'm thinking this PR will be in addition to many columns that could eventually be replaced by this PR, but I think that's outside the scope here.

Lastly, regarding the value of an attribute. I think it could be pretty easy to obtain the values for a resource given that resource's attribute fields. I'm mainly concerned about n+1s and making sure updating and fetching those attributes is performant. This is the main reason why attribute fields are denormalized and contains user_id.

[drusepth]

@mwerner Oh hey, I just saw the above! Thanks for the response.

I think your description of AttributeCategories and AttributeFields makes sense. Moving them to models and in the DB opens up showing/hiding them dynamically, which is also something some users have requested, so I think it's a good idea and you've got a good implementation of it here (and I'd love to see the hard-coded hashes in every model go away!). Good idea on the system fields as well.

Also, yeah, being performant is pretty important here, especially with the scaling of users we've seen since launch. Once you've got something working, I'd be happy to roll through the code and look for any optimizations I could make as well. I bet we could nail performance with a few passes. Also, gems like bullet are very useful locally for finding and fixing n+1's, if you haven't seen it already 👍

[mwerner]

Cleaned it up quite a bit. There are a few more things to consider:

• Needs tests
• I would like to clean up the value retrieval to allow for field.value. Primarily for performance, but it'd be good to make that more obvious how it's happening.
• I don't care for the AttributeCategory and AttributeField creation process. Not very intuitive.
• There's lots of potential to clean up the _panel.html.erb partial and moving a lot of that logic into the purview of AttributeField

I'm sure there are some other nuanced stuff like the question box in the sidebar and how these new content-ish resources interact with that, but I'll leave that to you guys. Just provide any feedback on the stuff that's here and we can figure out what we should keep to merge. Stuff like copy, icons and colors are all just placeholder without much thought put into it. Would love input on stuff like that to fit what you guys might have in mind.

[drusepth]

This is absolutely awesome (and thanks for the gif)!

Some notes on cleaning up the AttributeCategory and AttributeField creation process:

• Looks very powerful right now, but we probably want to abstract out a couple of those fields to make it more user-friendly. I think we can use some standardized icon for custom attribute groups, and make the name just a machine-friendly downcased-and-separated-by-underscores string of whatever the attribute label is.
• We can probably nest that form somewhere within each content type (e.g. a "settings" cog at the top of the Characters/Locations/Items page) that would autofill which entity type it's for. That'd keep the icons in the navbar from pushing out more (especially with new content types coming also), and make it feel more like a per-"character" (or whichever entity type) setting, instead of the global settings now that might have a bit of a learning curve (even though it'd be the same thing, just kind of abstracted out per entity type).
• I think with the above changes, we could have attribute categories in which all a user has to type in is the label (and just enforce uniqueness), which could make the UI for adding/removing new tabs very simple (probably as simple as clicking a [+] at the end of some content's tab strip and typing in new tabs).
• Similiarly on attributes, I think we should create the name from the label so users don't need to type in both.
• I think if we nest the form partial in at the end of each attribute category (behind some button to add more fields), we can also simplify the attribute form for the users to merely ask for the field label, making it a lot easier for any user to just add/remove fields without going through a big setup also.

It looks great so far though! I'm pretty sure this is one of the most-frequently-requested features. I'll take a look through the code now and first thing in the morning and see if I've got any comments as well.

Thanks again!

[drusepth]

Something like a gear here would be the best place to tuck away these settings for each content type, I think, if we don't end up with basically inline UI that lets users create tabs/fields directly from the content forms.

What do you think?

[drusepth]

Ha, this info is a bonus! Thanks!

[Cantido]

I don't know why the Travis build won't start, but it doesn't even appear in the build history so I can't force-restart it. If you could try merging master into your fork, then push again, that would probably get the build to go through.

[mwerner]

Unfortunately I'm going to be without internet for about a week or so. Feel free to hack on this branch if you like, otherwise I'll take a look at this when I get back.

[drusepth]

I didn't get a chance to dig as much into this branch as I'd hoped this week, but I'm going to finish up the relations on #133 and then try to dig in deep and get it merged into the content-expansion branch. It'd definitely be really awesome to have with that update.

[mwerner]

What do you think of this flow @drusepth, @Cantido

sorry for GoT spoilers btw

[Cantido]

That looks fantastic! The options drop-down is the perfect place for the feature, and I think the ability to create a new tab like that will be very appreciated. Also I love the refactor as a whole, it takes the app in a really good direction.

[drusepth]

@mwerner That looks almost exactly as how I was picturing it working, except even cleaner. Very awesome (except for the spoilers!). I think with a way to delete these fields, this could ship as-is and it'd provide a huge amount of value for all users. Is there anything other than that you were looking to add?

[mwerner]

How to delete the categories/fields is a good question. I'm not sure the best place to provide that functionality. Any thoughts?

[Cantido]

How about a small X icon at the top-right corner of each attribute field?

[mpigsley]

What about putting it in a three dot dropdown to the right of the field. That would make it easy to add extra functions in the future (e.g. attribute privacy).

[drusepth]

I think a three-dot dropdown is the best approach here. Also opens us up to do other manipulations as @mpigsley says, like adjusting per-field privacy (requested by users), being able to edit single fields without loading the whole edit page (requested by users), and other small things.

Could probably also throw in an edit link in that dropdown eventually so users could fix things like field typos in the same modal instead of having to delete and re-add fields.

Speaking of deleting fields, just to clarify @mwerner: if you delete a field with data in it, does that data disappear for all content using that field?

[mwerner]

The way it's written now, I'm grabbing all the attribute categories created by the current user for the current entity (Character, Location, etc). For each of those categories, I render the given field. So that means, if there are attribute values for that resource, they won't display. I think that could be changed without too much difficulty since the actual value itself is separate from the attribute field, but as it is right now, it just wouldn't show up.

I'll see what I can do about the per-field dropdown when i get some time to work on it tonight.

[drusepth]

I think that's alright, just wanted to scope out what the interaction was there. I would assume since they value and field are separate, that re-adding a deleted field with a value would actually being that value back? I think that's okay too.

Since this is branched off the master branch, I think the best course of action release-wise is to just release it when it's ready if it's ready before the content-expansion branch is, but we'll definitely want to figure out how to merge it to the expansion as well (which adds creatures, races, religions, and other content types). I haven't dug too much into the code here so I'm not sure how difficult that'd be, outside of porting over attributes to the new style here.

Again, great work! This is awesome, and something I can't wait to get out to everyone. 👍

[drusepth]

Not a showstopper, but I think the labels here are redundant with our internationalization en.yml (config/locales/en.yml). We'll probably want to eventually replace whatever uses these labels with the logic of:

1. If I18n.translate('characters.attributes.name') exists, use the label there
2. Otherwise (as I imagine would be the case for custom fields), just titleize the name here into a label

I think that'd let us have pretty sane defaults here without needing to specify a label for everything, while still allowing (language-agnostic) overrides that we can translate to the user's language. @Cantido Right?

[Cantido]

That's true, but since these values are no longer part of the ActiveModel object, I'm not sure how to leverage that machinery.

[mwerner]

I think the yaml files were just a way for me to clear up attribute_categories on the model. I think your best bet is to make everything beyond the very core system stuff like name be seeded on new accounts. That way if a user doesn't ever want to describe the appearance of something, they could in theory delete that whole category. Up to you guys if that's the direction you want to take it.

I also added some specs. I'm pretty happy with the state of the PR if you guys are. I think the only outstanding stuff is:

• Performance can probably be improved. There are a few places where it's being pretty lax about the types of queries it's requesting, but it's functional and I think cleaning up the field entry work is going to be a blocker on improving it much more.
• I had added content views to see a list of your attribute categories and fields. Those aren't referenced anymore now that we are using the dropdowns, but it felt like they may be useful at some point. Feel free to rip them out if you'd prefer the just not exist, but I'll leave that to you.
• The deleted attribute field value functionality stuff, which is just a matter of product decision.

@drusepth @Cantido @mpigsley

[mpigsley]

Should this be internationalized? Same with the confirmation below.

[mwerner]

Probably should be, but this is the only place I've seen in the repo that is doing any internationalization. I took this from actions_dropdown which isn't internationalized either. I think those types of changes should be done in a comprehensive way, I wouldn't want to dictate what helpers or patterns are defined for the implementation of i18n here.

[mpigsley]

Agreed 👍 Carry on

[Cantido]

I've kinda slowed down on #125 while these changes have been on the horizon, so that it doesn't get in your way while it's still half-baked. 😼

[drusepth]

This looks awesome and has my 👍 to go. I went ahead and merged it into the content-expansion branch (for a big release at once) and migrated over the new content types to your attribute system.

In its current state, I say lets 🚢 this bad boy. I've got a Medium post good to go and the branch looks good with this functionality. Great work all around, everyone. Happy to see this go live (soon)! 👏

[mwerner]

Sounds good. I'll leave this here for you to work in when/how ever you see fit.

Fun project, best of luck!

[drusepth]

We're live! @mwerner you've got a mention in the blog post for this feature -- will ping you when it's up :)