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

[WIP] Introduce the VM description editing page #8954

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeffibm
Copy link
Member

@jeffibm jeffibm commented Nov 3, 2023

app/controllers/vm_common.rb Outdated Show resolved Hide resolved
@agrare
Copy link
Member

agrare commented Nov 3, 2023

@jeffibm is there a supports feature check here?

@jeffibm
Copy link
Member Author

jeffibm commented Nov 6, 2023

@jeffibm is there a supports feature check here?

Hey @agrare , I replicated the same process for the edit page. Did not see a support feature check for that.

@jeffibm jeffibm force-pushed the edit-vm-description branch 2 times, most recently from 2cde17d to 83c6dd6 Compare November 6, 2023 05:14
@jeffibm
Copy link
Member Author

jeffibm commented Nov 6, 2023

Hey @kbrock , I was able to update the description with the old API. Is this alright?

Screen.Recording.2023-11-06.at.10.45.09.AM.mov

@kbrock
Copy link
Member

kbrock commented Nov 6, 2023

@jeffibm I see an example case using the api to edit a description: https://github.com/ManageIQ/manageiq-api/blob/master/spec/requests/vms_spec.rb#L1590

Is there a reason to not use this api and introduce on in ui-classic?

update:

url: `api/vms/#{record.id}`
payload: {"action"=>"set_description", "resource"=>{"new_description"=>"test"}}

@agrare
Copy link
Member

agrare commented Nov 6, 2023

I replicated the same process for the edit page. Did not see a support feature check for that.

I don't believe that means that we don't need one though. It looks like the previous edit page only changed attributes that live in our database, which means every providers' vms would support it. Setting the description on the underlying provider object is not the same.

]);

function createSchema(emsId, parentOptions, editDescription) {
console.log(editDescription);
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: remove this

@kbrock
Copy link
Member

kbrock commented Nov 9, 2023

class Vm
  supports :set_description
end

This works for vmware vms, templates, and ibm_power_hmc vms (where host_hmc_managed)

The privileges for this is the basic vm_edit, with a full path of all_vm_rules/vm_admin/vm_edit

@kbrock
Copy link
Member

kbrock commented Nov 13, 2023

If we keep the feature flag as vm_edit instead of vm_edit_description, I'm not sure if we need to do something special with privileges.

(which is different from the :set_description ems supports flag)

@miq-bot
Copy link
Member

miq-bot commented Nov 14, 2023

Checked commit jeffbonson@c987fd6 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
4 files checked, 16 offenses detected

app/controllers/application_controller/explorer.rb

app/controllers/vm_common.rb

app/helpers/application_helper/toolbar/x_vm_center.rb

app/views/vm_common/_form.html.haml

  • ⚠️ - Line 3 - Avoid using instance variables in partials views
  • ⚠️ - Line 3 - Layout/ExtraSpacing: Unnecessary spacing detected.
  • ⚠️ - Line 3 - Layout/ExtraSpacing: Unnecessary spacing detected.
  • ⚠️ - Line 3 - Layout/ExtraSpacing: Unnecessary spacing detected.
  • ⚠️ - Line 3 - Layout/ExtraSpacing: Unnecessary spacing detected.
  • ⚠️ - Line 3 - Layout/ExtraSpacing: Unnecessary spacing detected.
  • ⚠️ - Line 3 - Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.
  • ⚠️ - Line 3 - Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.
  • ⚠️ - Line 3 - Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.
  • ⚠️ - Line 3 - Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.
  • ⚠️ - Line 3 - Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.

@kbrock
Copy link
Member

kbrock commented Nov 29, 2023

@jeffibm did you say that the API was not working for you if the user had vm_edit privileges? Or maybe it was not displaying the entry in the dropdown?

@jeffibm
Copy link
Member Author

jeffibm commented Nov 30, 2023

@jeffibm did you say that the API was not working for you if the user had vm_edit privileges? Or maybe it was not displaying the entry in the dropdown?

If we try to use the same vm_edit in the menu section, it would complain about duplicate id being used in the actual "Edit this VM" button. So that made me create a new vm_edit_description.

@kbrock
Copy link
Member

kbrock commented Dec 2, 2023

@jeffibm did you say that the API was not working for you if the user had vm_edit privileges? Or maybe it was not displaying the entry in the dropdown?

If we try to use the same vm_edit in the menu section, it would complain about duplicate id being used in the actual "Edit this VM" button. So that made me create a new vm_edit_description.

Can we have 2 menu items that depend upon the same privileges. vm_edit is privileges, not a menu selection.

@miq-bot
Copy link
Member

miq-bot commented Mar 4, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

2 similar comments
@miq-bot
Copy link
Member

miq-bot commented Jun 10, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot
Copy link
Member

miq-bot commented Sep 16, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

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.

4 participants