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

Allow Zones to be edited, deleted #691

Merged
merged 3 commits into from
Oct 24, 2019
Merged

Allow Zones to be edited, deleted #691

merged 3 commits into from
Oct 24, 2019

Conversation

djberg96
Copy link
Contributor

Followup to #690, and very similar.

This PR allows for Zones to be edited and deleted. The only significant difference between this one and #690 is that previously some of these actions were explicitly disabled.

Partially addresses https://bugzilla.redhat.com/show_bug.cgi?id=1753806

@djberg96
Copy link
Contributor Author

@abellotti, @lpichler, @martinpovolny Please review.


expect(response).to have_http_status(:forbidden)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

nice test coverage !! 👍

@miq-bot
Copy link
Member

miq-bot commented Oct 23, 2019

Checked commits https://github.com/djberg96/manageiq-api/compare/831bba8a609bf95784bc0de26bd9c648b9abcbd1~...110b267f7c110ac0f2a4ec4c49b7df1f91528ded with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

super
end

private
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit concerned about this being the same as this:

https://github.com/ManageIQ/manageiq-api/pull/690/files#diff-a8c5eb840e76e0a1632c7acdb2b03a43R20

Well also the part above.

I think that the behavior should be moved to a/the? parent class or a mixin.

We can agree on merging this (and the 3rd one) and then refactor it once the functionality is there. If you prefer it that way.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fields actually aren't identical - created_at vs created_on. But yes, we should eventually try to put this into some common code.

@martinpovolny
Copy link
Member

ping @lpichler : merge?

@lpichler lpichler added this to the Sprint 123 Ending Oct 28, 2019 milestone Oct 24, 2019
@lpichler lpichler merged commit e9b1b2c into ManageIQ:master Oct 24, 2019
@abellotti
Copy link
Member

Hmm, I don't see where zones can be deleted in the UI unless I'm missing this. /cc @Fryguy

@djberg96 @martinpovolny I think we should forgo the delete, and shift to white-listing editable attributes here. gotta match the UI unless there are new UI requirements.

@abellotti
Copy link
Member

Looks like in the Settings.

image

though grayed out for current zone, what does API/Model return if the current zone is getting deleted ?

@djberg96
Copy link
Contributor Author

@abellotti There's some logic in zone.rb that prevents a zone in use from being destroyed:

https://github.com/ManageIQ/manageiq/blob/master/app/models/zone.rb#L243-L247

@simaishi
Copy link
Contributor

@djberg96 can this be ivanchuk/yes?

@djberg96
Copy link
Contributor Author

@martinpovolny or @abellotti any objections to making it ivanchuk/yes?

@simaishi
Copy link
Contributor

^ ping

@djberg96
Copy link
Contributor Author

Let's say yes. :)

simaishi pushed a commit that referenced this pull request Feb 21, 2020
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 97be56b07e6b6586b0921f4cf63b02464ee1b9bb
Author: Libor Pichler <lpichler@redhat.com>
Date:   Thu Oct 24 11:39:08 2019 +0200

    Merge pull request #691 from djberg96/zones_post_actions

    Allow Zones to be edited, deleted

    (cherry picked from commit e9b1b2c93d12d09bddf8dbdead5ada5dc3af8e90)

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

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.

6 participants