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

Edit VM Form - react #6748

Closed
himdel opened this issue Mar 9, 2020 · 20 comments · Fixed by #8072
Closed

Edit VM Form - react #6748

himdel opened this issue Mar 9, 2020 · 20 comments · Fixed by #8072

Comments

@himdel
Copy link
Contributor

himdel commented Mar 9, 2020

Converting Edit VM Form to React so that we can add authentications..
And also fix #6617.

API endpoint for editing: ManageIQ/manageiq#14623

@himdel himdel self-assigned this Mar 9, 2020
@himdel
Copy link
Contributor Author

himdel commented Mar 9, 2020

The current logic for available parents:

  def parent_choices(ids)
    @parent_choices = {}
    @parent_choices[Digest::MD5.hexdigest(ids.inspect)] ||= begin
      ems_ids = VmOrTemplate.where(:id => ids).distinct.pluck(:ems_id).compact
      parent_item_scope = Rbac.filtered(VmOrTemplate.where(:ems_id => ems_ids).where.not(:id => ids).order(:name))

      parent_item_scope.pluck(:name, :location, :id).each_with_object({}) do |vm, memo|
        memo[vm[0] + " -- #{vm[1]}"] = vm[2]
      end
    end
  end

  def parent_choices_with_no_parent_choice
    {'"no parent"' => -1}.merge(parent_choices(@record.id))
  end
= select_tag("chosen_parent",
                    options_for_select(parent_choices_with_no_parent_choice, @edit[:new][:parent]),

@himdel
Copy link
Contributor Author

himdel commented Mar 9, 2020

The logic for children:

Child VMs...

    vms = @record.children # Get the child VMs
    @edit[:new][:kids] = {}
    vms.each { |vm| @edit[:new][:kids][vm.name + " -- #{vm.location}"] = vm.id } # Build a hash for the kids list box

Available VMs...

vms = @record.children
...
@edit[:choices] = parent_choices([@record.id] + vms.pluck(:id))

@NickLaMuro
Copy link
Member

@himdel I assume that these ruby code examples from your two comments are what are available in manageiq-ui-classic, and some form of this functionality needs to exist in the API (if it doesn't already).

Correct?

@himdel
Copy link
Contributor Author

himdel commented Mar 9, 2020

What parent_choices does is return all VMs for the given providers, except for the VMs provided :).

So, available parents currently doesn't care if the VM is also in children,
and for available children, we only care about skipping the VMs already in .children

So, to keep the existing logic, we only need:

  • edited VM's provider id (available as .ems_id in /api/vms/1)
  • edited VM's children ids
  • all VMs under that provider (/api/vms/?filter[]=ems_id=55&expand=resources)

@himdel
Copy link
Contributor Author

himdel commented Mar 9, 2020

@NickLaMuro exactly the point .. I think we already have everything, iff I'm reading that right and if we want to keep the logic as is.

(Can you double check my logic please? :))

@NickLaMuro
Copy link
Member

(Can you double check my logic please? :))

Yup, no problem. Just saw this come in and wanted to make sure I understood what was being referenced here, since @skateman pinged me about this earlier today.

Will be looking into this a bit more this evening, and will report back here then. 👍

@himdel
Copy link
Contributor Author

himdel commented Mar 9, 2020

Perfect, thanks! :)

@himdel
Copy link
Contributor Author

himdel commented Mar 9, 2020

Assuming my logic works, there are only 2 problems:

  • no way to get a list of VMs children via the API (tried /api/vms/1?expand=children or attributes=children)
  • no way to get a VMs parent via the API (.parent_id is not there)

(And one note - archived/orphaned VMs should have that part disabled, no ems_id.)

@NickLaMuro
Copy link
Member

Okay, sounds feasible if it isn't already. I admit it takes me a while to find what is and isn't already in the API, so it might take me a "hot minute" to determine what is available.

@himdel
Copy link
Contributor Author

himdel commented Mar 9, 2020

Yeah, me too, especially for custom-implemented things..

Such as this case :) ... /api/vms/1?attributes=child_resources,parent_resource does give me both children and parent (when present).

@himdel
Copy link
Contributor Author

himdel commented Mar 9, 2020

@NickLaMuro but I think the current code may be wrong...

manageiq/app/models/vm_or_template.rb
virtual_has_many   :child_resources,        :class_name => "VmOrTemplate"
virtual_has_one   :parent_resource,      :class_name => "VmOrTemplate"


  def child_resources
    children
  end

  def parent_resource
    parent
  end

There's no RBAC.filtered involved anywhere. Unless the API somehow understands that that particular virtual attribute is a collection and needs rbac filtering.

@NickLaMuro
Copy link
Member

@himdel it should, but I would have to check... pretty sure all attributes in the API are Rbac.filtered, but want to confirm.

Let me look into it.

@NickLaMuro
Copy link
Member

NickLaMuro commented Mar 10, 2020

@himdel Okay...

As a quick primer, the attributes from your query above are accessible in the controller thanks to these two methods:

https://github.com/ManageIQ/manageiq-api/blob/cd849c1/lib/api/request_adapter.rb#L36-L38
https://github.com/ManageIQ/manageiq-api/blob/cd849c1/app/controllers/api/base_controller/parameters.rb#L50-L58

And the part that filters it is from the base_controller/renderer.rb module, which the important bits are the following two sections:

https://github.com/ManageIQ/manageiq-api/blob/cd849c1/app/controllers/api/base_controller/renderer.rb#L79
https://github.com/ManageIQ/manageiq-api/blob/cd849c1/app/controllers/api/base_controller/renderer.rb#L201-L219

So I think we should be good, assuming the child_resources/parent_resource are considered virtual_attributes, but I am pretty sure the RelationshipMixin in manageiq would have it treated as virtual_attributes.

(to be fair, I think the code that determines if it is "virtual" basically checks if it is a relation as well, so that should be any non-column/non-vanilla-ruby-method method on an ActiveRecord object... but I will check that as well)

Edit: Oh, nevermind, these are straight up virtual_* based relations:

https://github.com/ManageIQ/manageiq/blob/a59c2aa/app/models/vm_or_template.rb#L164
https://github.com/ManageIQ/manageiq/blob/a59c2aa/app/models/vm_or_template.rb#L170

So these will definitely be picked up by this:

https://github.com/ManageIQ/manageiq-api/blob/cd849c1/app/controllers/api/base_controller/renderer.rb#L350-L356

@himdel
Copy link
Contributor Author

himdel commented Mar 10, 2020

Aah, thanks! Yeah I see it now :)

So, TLDR: we're good on API side (assuming everything works as described)

@rvsia
Copy link
Contributor

rvsia commented Mar 11, 2020

@Hchrast has this form done #5862, just missing tests and cleanup edit: nvm, you know it, sorry :D

@himdel
Copy link
Contributor Author

himdel commented Mar 30, 2020

Current state - Explorers:

Compute > Infrastructure > Virtual Machines...
toolbar Configuration > Edit Selected Item ("Edit this VM/Template" on detail screen)

Vm:

before_explorer_vm

Template:

before_explorer_template

Compute > Clouds > Instances...
toolbar Configuration > Edit Selected Item ("Edit this Instance/Image" on detail screen)

Instance:

before_explorer_instance

Image:

before_explorer_image

(added green border to show #main_div, which is the whole vm_common/form partial)

@himdel
Copy link
Contributor Author

himdel commented Mar 30, 2020

Current state - non-explorers:

Compute > Infrastructure > Providers
pick a provider, choose VMs / Templates (/ems_infra/22?display=miq_templates or =vms),
toolbar Configuration > Edit Selected Item

VM:

before_bare_vm

Template:

before_bare_template

Compute > Clouds > Providers
pick a provider, choose Instances / Images (/ems_cloud/38?display=images or =instances),
toolbar Configuration > Edit Selected Item

Instance:

before_bare_instance

Image:

before_bare_image

@NickLaMuro
Copy link
Member

"Ride Of The Valkyries" eh. 😏

@himdel
Copy link
Contributor Author

himdel commented Mar 31, 2020

well, nothing beats the classics :)

@himdel
Copy link
Contributor Author

himdel commented Mar 31, 2020

More current state .. Services > Workloads, both accordions
toolbar Configuration > Edit this * / Edit Selected item

VMs & Instances:

before_explorer_vot_vai

Templates & Images:

before_explorer_vot_tai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants