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

Fix based_volumes sublist #2094

Merged
merged 2 commits into from
Sep 15, 2017
Merged

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Sep 5, 2017

Fix based_volumes sublist.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1449294

And also new bug introduced by doing 2nd rendering of the table using report_data

Fixed based_volumes sublist for the new report_data function
@Ladas
Copy link
Contributor Author

Ladas commented Sep 5, 2017

before:

throws an exception

after:

screenshot from 2017-09-05 08-24-47

@miq-bot
Copy link
Member

miq-bot commented Sep 5, 2017

Checked commits Ladas/manageiq-ui-classic@dac84e8~...f67b0d1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@Ladas
Copy link
Contributor Author

Ladas commented Sep 5, 2017

@martinpovolny @karelhala are we working on the issue of not rendering the tables more than once? Now it goes all the way through the generic_show_mixing, then there seems to be a new way using report_data method, that uses new mappings for exceptions the generic mixin was trying catch. And since we just turned this by default, more pages might be broken, e.g. all that are using a specific association
https://github.com/Ladas/manageiq-ui-classic/blob/36c5a957b6fcad6a89d81bb8238c70ef04a55d1e/app/controllers/mixins/generic_show_mixin.rb#L151 or even model.

Also I was trying to do at least basic integration specs for rendering of the tables, e.g. https://github.com/Ladas/manageiq-ui-classic/blob/48830b68883f97eab15533375474b8dca8992e39/spec/shared/controllers/shared_examples_for_network_port_controller.rb#L48 , do we have something similar for the report_data method? Or it's all untested again? Also those table specs should have been enhance to test e.g. filtering and ordering. These are the parts we are breaking over and over again. :-(

@martinpovolny
Copy link
Member

should we add the assert_nested_list test?

@Ladas
Copy link
Contributor Author

Ladas commented Sep 12, 2017

@martinpovolny we should add the test indeed :-)

@mzazrivec mzazrivec self-assigned this Sep 15, 2017
@mzazrivec mzazrivec added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 15, 2017
@mzazrivec mzazrivec merged commit ff8407a into ManageIQ:master Sep 15, 2017
@simaishi
Copy link
Contributor

@Ladas MODEL_STRING doesn't exist in Fine branch and I'm not sure where the changes should go. Please create a PR for Fine branch.

@Ladas
Copy link
Contributor Author

Ladas commented Nov 14, 2017

@simaishi #2731

@simaishi
Copy link
Contributor

Backported to Fine via #2731

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