Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config is reset #515

Closed
pivotalneutron opened this issue Jul 4, 2011 · 16 comments
Closed

Config is reset #515

pivotalneutron opened this issue Jul 4, 2011 · 16 comments
Assignees
Milestone

Comments

@pivotalneutron
Copy link

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
Copy link
Collaborator

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

@bbenezech
Copy link
Collaborator

reverted.

@kaapa
Copy link
Collaborator

kaapa commented Jul 4, 2011

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

@bbenezech
Copy link
Collaborator

Yep.

@ghost ghost assigned kaapa Jul 10, 2011
@kaapa
Copy link
Collaborator

kaapa commented Jul 11, 2011

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
Copy link
Collaborator

kaapa commented Jul 11, 2011

@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
Copy link
Collaborator

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

@sferik
Copy link
Collaborator

sferik commented Jul 12, 2011

👍 Nice work on this!

@bbenezech
Copy link
Collaborator

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

@kaapa
Copy link
Collaborator

kaapa commented Jul 13, 2011

@bbenezech good catch! Now fixed.

@sferik
Copy link
Collaborator

sferik commented Jul 13, 2011

Is this ready to merge?

@kaapa
Copy link
Collaborator

kaapa commented Jul 14, 2011

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

@sferik
Copy link
Collaborator

sferik commented Jul 14, 2011

@kaapa Go for it!

@sferik sferik closed this as completed Jul 14, 2011
@bbenezech bbenezech reopened this Aug 10, 2011
@bbenezech
Copy link
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
Copy link
Collaborator

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

@bbenezech
Copy link
Collaborator

stalled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants