search

zinc-collective / convene

An Operating System for the Solidarity Economy
https://convene.zinc.coop
Other
56 stars 19 forks source link

Update ERD generation method and style #1042

Closed daltonrpruitt closed 1 year ago

daltonrpruitt commented 1 year ago

After looking at some different attributes of the Rails ERD generator, this one seemed to help visually the most without removing the data members. The actual choice is still up for discussion, and I figured we'd actually update the ERD when we do decide on the best design.

daltonrpruitt commented 1 year ago

Some Options:

Connected-components only (the one I liked initially): erd_inheritance_only

Vertical (...probably not): erd_inheritance_vertical

Simplified (i.e. just names, no data members) : erd_simplified

With bachman notation to show cardinality (not sure if this is necessary): erd_inheritance_bachman

There were many more options, but I went with some I thought were interesting. Different combos can be tried (I think most of these used the connected-only anyways, so if we want the unconnected ones included, let me know).

anaulin commented 1 year ago

Thanks for taking a crack at improving this, @daltonrpruitt !

I personally don't have strong opinions on this either way, if "connected" is what seems most helpful to y'all, that works for me too.

zspencer commented 1 year ago

Thanks for digging into this! I think getting our ERD generation playing nicely will really be helpful for people trying to navigate the breadth of a domain model that encompasses both Operating System and Application concerns.

Re: Connected - I think we still want the unconnected ones, because we still want to demonstrate that the Livestream Furniture is a piece of the Convene architecture.

That said, I remain confused about why Livestream, Marketplace::Marketplace and Journal::Journal don't indicate that they are descendants of Furniture. Is it because we're not using Rails Single Table Inheritance directly? What does it look like if we set polymorphism or inheritance to true? What about the cluster option?

daltonrpruitt commented 1 year ago

Using all three of those you mentioned (i.e. --polymorphism --inheritance --cluster), the output has some more information, but still does not show the Furniture module. erd_poly_inher_cluster

Is it possible the rails-erd does not count modules when determining inheritance structure? I don't know enough about what's going on in the backend to say if that's even relevant, but a bit more snooping showed that everything (other than the ActiveStorage stuff, which seems to come from schema.rb) are classes that inherit from ApplicationRecord, which seems to be standard for these models.

As @zspencer noted, there are some (Journal::Journal, EmbeddedForm, Livestream, MarkdownTextBlock, VideoBridge, and Marketplace::Marketplace) that don't show the actual connection they have, FurniturePlacement. While FurniturePlacement does inherit from ApplicationRecord, these specific classes do not, as shown in the above diagram. So, I assume your guess is correct: this is because of not using the Single Table Inheritance directly.

That being said, I don't know how to change that without changing the schema (not that I definitely know how to do that safely), and that's probably not a good idea to just fix the diagram.

zspencer commented 1 year ago

Agreed; I don't want to get caught up in significant schema changes... I think I like this a little bit better just because it shows the designs flaws a bit more clearly. That said, I wish we could ditch the ActiveStorage/ActionText/etc. namespaces. Maybe it's time to sprout a configuration file for the diagram so bundle exec rails-erd applies the settings we want?

I'm also happy to merge once the instructions for updating the erd include the new flags.

zspencer commented 1 year ago

Oh! And I mis-spoke; re: Furniture module; I would like it to sow that Journal::Journal inherit from FurniturePlacement...

I wonder what would happen if FurniturePlacement didn't inherit from ApplicationRecord at all; and instead inherited from ActiveRecord::Base similar to how ApplicationRecord does already?

daltonrpruitt commented 1 year ago

Accidentally closed the request when I tried changing the name of the branch; I didn't realize it would close. My bad.

I messed with the FurniturePlacement inheritance, but changing it to ActiveRecord::Base just removed the link from FurniturePlacement to ApplicationRecord.

See the new commits today for the config file and the resulting ERD. Using the exclude option allows the removal of ActiveStorage/ActionText/etc. It turns out specifying the orientation as horizontal (which is the default...) in this file results in the attributes showing up on the right side of the class names (which looks pretty bad).

daltonrpruitt commented 1 year ago

Ready to merge. Don’t know why it was still a draft other than second guessing myself. 🤷‍♂️

zspencer commented 1 year ago

Merge-arita!