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

ApplicationRecord - temporarily add .decorate and self.decorate #14093

Merged
merged 1 commit into from
Feb 28, 2017
Merged

ApplicationRecord - temporarily add .decorate and self.decorate #14093

merged 1 commit into from
Feb 28, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Feb 28, 2017

This is UI code that should live in the UI, but that requires a few more steps first.

In the mean time, we either have to keep patching ApplicationRecord and deal with the autoloader, or just add it to ApplicationRecord directly :).

This is a follow up to ManageIQ/manageiq-ui-classic#237 (which solved this by an initializer, which doesn't work with rails' autoloading in development mode).


Please merge together with ManageIQ/manageiq-ui-classic#506 (either will break without the other).

@miq-bot add_label technical debt, ui, euwe/no

This is UI code that should live in the UI, but that requires a few more steps first.

In the mean time, we either have to keep patching ApplicationRecord and deal with the autoloader, or just add it to ApplicationRecord directly.
@himdel
Copy link
Contributor Author

himdel commented Feb 28, 2017

Ping @Fryguy , @chrisarcand, @martinpovolny .. this is meant as a temporary solution for the autoloader issues, which will get removed in the second round of decorator refactoring.

(Unfortunately can't do that right away, since we're blocked by #14040 for now.)

@miq-bot
Copy link
Member

miq-bot commented Feb 28, 2017

Checked commit https://github.com/himdel/manageiq/commit/86c9a35bc303287194a8957b931d80177265ef12 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍪

@martinpovolny
Copy link
Member

martinpovolny commented Feb 28, 2017

I understand the concerns about decorating models for the sake of UI and altering ApplicationRecord. But it will take some time before we are able to untie the frontend from the backend and before we do that we have a log of cleaning work to do in the UI.

To move forward we need this.

Unless someone is strongly againts this (and can help in finding a better solution) I'd merge this.

Copy link
Member

@chrisarcand chrisarcand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine temporarily, to me. 🙏 Please let it be only temporary; as discussed, true decoration really should not need anything like this.

👍

@chrisarcand chrisarcand requested a review from Fryguy February 28, 2017 15:17
@Fryguy Fryguy merged commit de04f9a into ManageIQ:master Feb 28, 2017
@Fryguy Fryguy added this to the Sprint 56 Ending Mar 13, 2017 milestone Feb 28, 2017
@himdel himdel deleted the move-decorator branch February 28, 2017 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants