Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Config is reset #515

Closed
pivotalneutron opened this Issue · 16 comments

4 participants

@pivotalneutron

Our configuration is applied then reset when our app starts. The culprit is the to_prepare method in lib/rails_admin/engine.rb which runs after the configuration for some reason. It is added in this commit:

d035100#diff-2

@bbenezech
Collaborator

+1 same here.
I'm reverting both branches.
@kaapa can you have a look?
Conf simply doesn't load.

@bbenezech
Collaborator

reverted.

@kaapa
Collaborator

Thank's for the report - I'll take a look. I suppose we're talking about model specific configurations within an initializer here?

@bbenezech
Collaborator

Yep.

@kaapa kaapa was assigned
@kaapa
Collaborator

So that's my prototype for the fix. It includes moving all configurables under RailsAdmin::Config which is able to reset itself to a vanilla state. Also RailsAdmin.config now stores all blocks to a registry so we're able to reapply initializer code post-reset. Configurations through ActiveRecord::Base.rails_admin are not cached so changes to model configurations apply without restart in development mode.

@kaapa
Collaborator

@bbenezech, @gunn, @sferik and anyone willing to review - I think I've got it together now.

To recap on this feat; The intention is to provide lazy loading of configurations so that model classes and all that jazz don't get eager loaded for example when running rake tasks. It started with the idea of having model configurations done within model classes so that they wouldn't cause the premature loading which in turn caused some issues running migrations.

That was all nice, but as this issue states model configurations done in initializers got lost during reset. Now the reset actually wasn't required by the original feature, but I thought it would be nice to have as it has been requested at some point - also having reset mechanism allows the tests to be better isolated and development cycle becoming bit faster (as model config changes become live). Wanting to keep the reset I implemented initializer calls to RailsAdmin.config being stored in an array so they can be re-evaluated after each reset.

At some point when crafting the reset to be more exhaustive I came across the fact that some of our default configuration is stored in other places than RailsAdmin::Config. I've updated the readme API notes section with the changes. The idea is to have all non-model config in one place so reset will be easier to maintain. Also I believe RailsAdmin::Config is the most logical place for them.

Lastly I remembered we have batch accessors at config's disposal. Using any of those would render the lazy loading dysfunctional so I implemented RailsAdmin.setup which invokes the stored initializer blocks as late as possible. I decided to use ActionDispatch´s before callback for setup and after callback for reset. Those are run on dispatch and not directly after startup (rake tasks invoke to_prepare which was my initial callback). I haven't tested, but I believe this may also improve application startup time.

As you see there's a lot going on here and I definitely need a code review. Please check it out and lets see how to proceed from there.

Cheers!

@bbenezech
Collaborator

Thanks for all the hard work you've done, it's awesome. I'll cherry-pick and try it.

@sferik
Owner

:+1: Nice work on this!

@bbenezech
Collaborator

@kaapa Can you check it against cancan? It blows a stacktrace on _current_user (in the controller mixin)

@kaapa
Collaborator

@bbenezech good catch! Now fixed.

@sferik
Owner

Is this ready to merge?

@kaapa
Collaborator

I consider this ready for merge - shall I do it?

@sferik
Owner

@kaapa Go for it!

@sferik sferik closed this
@bbenezech bbenezech reopened this
@bbenezech
Collaborator

We're seeing a lot of problem with the new configuration style. I need to take a serious look at it. Since the Readme has been rewritten toward this new design, newcomers are facing issues that cannot easily be solved.

@bbenezech
Collaborator

Targetting next release. 0.0 will be initializer only, I guess.

@bbenezech
Collaborator

stalled

@bbenezech bbenezech closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.