mountetna / magma

Data server with friendly data loaders
GNU General Public License v2.0
5 stars 2 forks source link

Make chosen attribute options editable #123

Closed alimi closed 4 years ago

alimi commented 4 years ago

Per #99, this PR adds a global attributes table that stores attribute options desc, display_name, match, and format_hint. Options will be fetched from the attributes table for a project/model/attribute if it exists; otherwise, we'll continue to use the definition in the model file.

This PR also adds a global_migrate command to run database wide migrations. It could be merged into migrate, but handling the version flag could be weird. How would we tell if the requested version is for a global migration or a project specific migration?

This is our first pass through this problem -- we're happy for all feedback! 😄

/cc @notmarkmiranda

graft commented 4 years ago

How would we tell if the requested version is for a global migration or a project specific migration?

I think this is already a problem for the existing migrate command, which takes a single version argument and applies it to all projects. It's only been ok because there is a single project in production.

In the branch graft-add-load I wrote a version of this command that handles global migrations, with an interface like: [<project_name> [<version_number>]]. No arguments would migrate everything to latest; global migrations were run using the project_name 'magma'.

However, since we're hoping to axe the project-specific migrations and only retain the global migrations, i think it's best to keep the two commands separate.

graft commented 4 years ago

Magma::Attribute.update_option inserts into the attribute table - what happens after subsequent calls to update_option?

Also while your spec tests update_option for its ability to change the json_template, you also set the @#{opt} in fetch_value which I think does not get exercised here.

alimi commented 4 years ago

Magma::Attribute.update_option inserts into the attribute table - what happens after subsequent calls to update_option?

Good catch! In https://github.com/mountetna/magma/pull/123/commits/accaa0d99fcb22248eb6c0cf4344d659938c8f47, I added a unique index on the attributes table so it'll only have one row per project/model/attribute. Magma::Attribute#update_option will now do an upsert - it'll insert if there's no attributes row per project/model/attribute; otherwise, it'll do an update.

Also while your spec tests update_option for its ability to change the json_template, you also set the @#{opt} in fetch_value which I think does not get exercised here.

I think you're referring to instance_variable_set("@#{opt}", new_value) in Magma::Attribute#update_option. Without setting @#{opt}, the test fails. Before the test runs, spec/spec_helper.rb starts the Magma::Server which will eventually cause all the models (and their attributes) to be loaded into memory. Attribute options only get set once when models/attributes are loaded. So when the test inserts new values into the attributes table, they never show up in the already loaded models. That's why we added Magma::Attribute#update_option - it can update the already loaded attribute options be setting @#{opt}.

It might be nice to have a test that verifies we're fetching attributes out of the db, but we couldn't find a way around the models already being loaded.

alimi commented 4 years ago

Since the match option is more configurable than the other editable attribute options (desc, display_name, and format_hint), we've removed support for editing it in https://github.com/mountetna/magma/pull/123/commits/751b23ac44144589d685e0ec84668fb3d14ca020.

graft commented 4 years ago

I have only one tiny request, which is: can we change desc to description? I've long despised that. Otherwise :ship:

alimi commented 4 years ago

@graft yeah, let's change it! I guess we'll also have to update the IPI models to use description too?

alimi commented 4 years ago

Ok, I replaced desc with description. desc can still be set on model attributes as a fallback. This will let us support stuff like IPI models until they've been updated. (https://github.com/mountetna/magma/pull/123/commits/ed5cbe2a58a7b6d43d7b88492f4f37942008fc7a)

I also left desc as the key in Magma::Attribute#json_template so the API/client will keep working.

alimi commented 4 years ago

I did a little clean up in https://github.com/mountetna/magma/pull/123/commits/3f48b38c7da96e41b3462fa8e4ab21001a3c5863. Before, attribute options would only be set from the database if the attribute was defined with some options in Ruby. Now, attribute options will be set from the database whether or not they're defined in Ruby.

graft commented 4 years ago

:+1: :shipit: