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

Drop unused views with ems_common/angular/form references #7607

Merged
merged 1 commit into from
Feb 1, 2021
Merged

Drop unused views with ems_common/angular/form references #7607

merged 1 commit into from
Feb 1, 2021

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jan 28, 2021

not used since the provider form rewrite, removing.

(And removed from the list in #7603 :))

not used since the provider form rewrite, removing
@himdel himdel added the cleanup label Jan 28, 2021
@miq-bot
Copy link
Member

miq-bot commented Jan 28, 2021

Checked commit https://github.com/himdel/manageiq-ui-classic/commit/615baa2f3df889ec3516bd302d0d770966cb2768 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
4 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby26, which recognizes
warning: 2.6.6-compliant syntax, but you are running 2.6.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@himdel himdel mentioned this pull request Jan 28, 2021
@chessbyte chessbyte assigned chessbyte and skateman and unassigned chessbyte Jan 28, 2021
@skateman
Copy link
Member

🤔 I'm wondering if we still need the routes/methods for these views?

Maybe I could update my magic specs to also catch this, meaning if there is a route and a matching controller method, it should have a view as well (at least for the generic ones like new, edit, view, etc...) cc @Fryguy

@himdel
Copy link
Contributor Author

himdel commented Jan 28, 2021

Not sure I follow? The url of that form is /ems_storage/new (or edit/123, or block storage). So the routes and methods still exist, but they render DDF instead of the same name template.

@skateman
Copy link
Member

For me it seems like none of the forms you're deleting are being used, they are exposed but not accessible through our UI. It's not true that they have been converted to DDF, only the ems_block_storage/new|edit has been converted which is not being affected by your changes.

Just try to access the matching URLs of the views you're trying to delete.

@himdel
Copy link
Contributor Author

himdel commented Feb 1, 2021

So.. are you seeing some error somewhere? Or what are you trying to say?

@himdel
Copy link
Contributor Author

himdel commented Feb 1, 2021

Or are you saying those forms should still be using ems common and therefore it should not have been deleted?

@skateman
Copy link
Member

skateman commented Feb 1, 2021

No, I'm saying we can also drop some controller methods and routes related to these views...

@himdel
Copy link
Contributor Author

himdel commented Feb 1, 2021

Ah, ok 👍 , let's leave that for a separate PR then, that sounds like it will need some digging to find out what's still used and what isn't.

@skateman skateman merged commit d6e5041 into ManageIQ:master Feb 1, 2021
@himdel himdel deleted the rm-ems branch February 1, 2021 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants