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] Add WidgetSet endpoint #968

Closed
wants to merge 1 commit into from

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Nov 19, 2020

Fixes #953

This adds WidgetSet endpoint with CRUD.

LIST

GET /api/widget_sets
=>
{
    "name": "widget_sets",
    "count": 8,
    "subcount": 8,
    "pages": 1,
    "resources": [
        {
            "href": "http://localhost:3090/api/widget_sets/155"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/161"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/162"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/168"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/170"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/152"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/153"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/154"
        }
    ],
    "actions": [
        {
            "name": "query",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "create",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "edit",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "delete",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "copy",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "delete",
            "method": "delete",
            "href": "http://localhost:3090/api/widget_sets"
        }
    ],
    "links": {
        "self": "http://localhost:3090/api/widget_sets?offset=0",
        "first": "http://localhost:3090/api/widget_sets?offset=0",
        "last": "http://localhost:3090/api/widget_sets?offset=0"
    }
}

CREATE:

POST /api/widget_sets
{
    "name": "V",
    "description": "a",
    "set_data": { "col1": [7, 22, 20] },
    "group"   : {"id" : 1} # or href
}
=>
 {
    "results": [
        {
            "href": "http://localhost:3090/api/widget_sets/167",
            "id": "167",
            "name": "V",
            "description": "a",
            "set_type": "MiqWidgetSet",
            "created_on": "2020-12-09T15:29:35Z",
            "updated_on": "2020-12-09T15:29:35Z",
            "guid": "847f3d4f-9ccf-4049-9b60-d40a3d755bbd",
            "read_only": null,
            "set_data": {
                "col1": [
                    7,
                    22,
                    20
                ],
                "col2": [],
                "col3": [],
                "reset_upon_login": false,
                "locked": false
            },
            "mode": null,
            "owner_type": "MiqGroup",
            "owner_id": "1",
            "userid": null,
            "group_id": "1"
        }
    ]
}

EDIT:


POST /api/widget_sets/1
{
   "action" : "edit",
   "resource" : {
        "description": "testaa",
        "set_data": { "col1": [7, 22, 20] }
   } 
}

COPY

POST /api/widget_sets/167
{
   "action" : "copy",
   "resource" : {
        "name" : "ddd",
        "description": "testaa",
        "set_data": { "col1": [7, 22, 20] }
   } 
}
=>
{
    "success": true,
    "message": "Dashboard ddd successfully created.",
    "href": "http://localhost:3090/api/widget_sets/167"
}


DELETE

POST /api/widget_sets/167
{
   "action" : "delete"
}
=>

{
    "success": true,
    "message": "Dashboard Vd has been successfully deleted.",
    "href": "http://localhost:3090/api/widget_sets/169"
}

Links

Requires:

@skateman
Copy link
Member

skateman commented Dec 2, 2020

@lpichler
Copy link
Contributor Author

lpichler commented Dec 2, 2020

@lpichler we also need an endpoint for copying I think

https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/report_controller/dashboards.rb#L61

It could be done by with create action but I can add it to simplify copy action.

@lpichler lpichler changed the title Add WidgetSet endpoint [WIP] Add WidgetSet endpoint Dec 2, 2020
@miq-bot miq-bot added the wip label Dec 2, 2020
@skateman
Copy link
Member

skateman commented Dec 2, 2020

@lpichler as there's a separate method on the model for copying, I'd rather expose it than simulate it with a create...

@lpichler lpichler force-pushed the add_widget_set_endpoint branch 3 times, most recently from 0103ab1 to adea740 Compare December 9, 2020 15:55
@lpichler lpichler changed the title [WIP] Add WidgetSet endpoint Add WidgetSet endpoint Dec 9, 2020
@lpichler lpichler removed the wip label Dec 9, 2020
@lpichler lpichler mentioned this pull request Dec 9, 2020
10 tasks
@Fryguy
Copy link
Member

Fryguy commented Dec 9, 2020

I'm not sure I like all of this in the API...feels like a lot of the setup should be in the model as class methods. @agrare @abellotti Thoughts? Is this a pattern we do elsewhere?

ALLOWED_FIELDS = REQUIRED_FIELDS_TO_COPY + %w[group description guid read_only set_data].freeze
SET_DATA_COLS = %i[col1 col2 col3].freeze

def init_set_data(set_data)
Copy link
Member

Choose a reason for hiding this comment

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

Anything that's not exposed to a route should be moved into a private method.


def validate_description(group_id, description)
widget_set_exists_in_group = MiqWidgetSet.exists?(:owner_id => group_id, :description => description)
raise ArgumentError, "Description(Tab Title) must be unique for this group" if widget_set_exists_in_group
Copy link
Member

Choose a reason for hiding this comment

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

Like this stuff should be model validations...not in the API.

def create_resource(type, id = nil, data = nil)
raise_if_unsupported_fields_passed(data)

raise ArgumentError, "Name cannot contain \"|\"" if data['name'].index('|')
Copy link
Member

Choose a reason for hiding this comment

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

Also should be a model validation.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, API does not raise any ArgumentErrors today, those come from the model.

@lpichler
Copy link
Contributor Author

lpichler commented Dec 9, 2020

I'm not sure I like all of this in the API...feels like a lot of the setup should be in the model as class methods. @agrare @abellotti Thoughts? Is this a pattern we do elsewhere?

Yes, this is exactly what I wanted to discuss - I am not against, I wanted to see opinions from others.
This is common that there is lot of validations of various kinds in controllers and when we are creating API endpoints we have to decide if validations should go to in API or to model level(and we can rely that data in db are already valid ).

cc @kbrock

new_set_data[:reset_upon_login] = !!set_data['reset_upon_login']
new_set_data[:locked] = !!set_data['locked']
new_set_data
end
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused with this, is the API defining a structure beyond the basic MiqWidgetSet ? It almost feels we need a new subclass of MiqWidgetSet in the model for what we're building/managing here and move most of this logic in there.

@lpichler lpichler changed the title Add WidgetSet endpoint [WIP] Add WidgetSet endpoint Dec 10, 2020
@miq-bot miq-bot added the wip label Dec 10, 2020
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 16, 2020
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 16, 2020
@lpichler
Copy link
Contributor Author

@miq-bot cross-repo-tests ManageIQ/manageiq#20890

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 16, 2020
@miq-bot
Copy link
Member

miq-bot commented Dec 16, 2020

Checked commit lpichler@4dea6a4 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 4 offenses detected

app/controllers/api/widget_sets_controller.rb

@gtanzillo gtanzillo assigned Fryguy, gtanzillo and abellotti and unassigned gtanzillo Jan 20, 2021
module Api
class WidgetSetsController < BaseController
REQUIRED_FIELDS_TO_COPY = %w[name].freeze
ALLOWED_FIELDS = REQUIRED_FIELDS_TO_COPY + %w[group description guid read_only set_data].freeze
Copy link
Member

Choose a reason for hiding this comment

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

with 'read_only' here, does this mean the API can create read_only widget sets and cannot delete them ? I usually think of read_only as set to true for some system seeded resources.

@abellotti
Copy link
Member

needs a bulk query test. can be added in spec/requests/collections_spec.rb

@miq-bot
Copy link
Member

miq-bot commented Feb 16, 2021

This pull request is not mergeable. Please rebase and repush.

@chessbyte chessbyte mentioned this pull request Feb 23, 2021
7 tasks
@lpichler lpichler closed this Jul 13, 2021
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jul 13, 2021
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jul 13, 2021
@kavyanekkalapu
Copy link
Member

@jrafanie This is needed for Overview/Reports/Dashboards -> Copy Selected Dashboard Missing endpoint for manipulating with dashboards form. Right now we are using miqajaxmethod as we don't have api.

@kbrock kbrock assigned kbrock and unassigned gtanzillo and abellotti Dec 3, 2021
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.

Missing endpoint for manipulating with dashboards
8 participants