Union edit service_dialog API call with other calls #285

paulslaby · 2018-01-16T09:59:46Z

This change is communicated with @simaishi at ManageIQ/manageiq#16747.
This change should be backported to fine. Follow the same link as above to find the change.

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

paulslaby · 2018-01-16T10:01:28Z

@simaishi I cannot add labels. So please do it yourself. Only bot can do it according to the PR name, and i do not think it is a nice way to do it.

simaishi · 2018-01-16T14:43:31Z

@paulslaby Added the labels. Once this PR is merged, I'll backport to Gaprindashvili and Fine branches.

abellotti · 2018-01-16T14:45:30Z

app/controllers/api/service_dialogs_controller.rb

-        service_dialog.update_tabs(data['content']['dialog_tabs']) if data['content']
-        service_dialog.update!(data.except('content'))
+        service_dialog.update!(data.except('dialog_tabs', 'content'))
+        service_dialog.update_tabs(data['dialog_tabs'] || data['content']['dialog_tabs']) if data['dialog_tabs'] || data['content']


What if both are specified, what is the expectation.

specs needed to test both signatures.

/cc @jntullo

If both are specified, the one without 'content' takes precendence.
Maybe warning about it?

I think we should log a warning, maybe mentioning something like "Both 'dialog_tabs' and 'content' : 'dialog_tabs' were specified. 'content' : 'dialog_tabs' will be ignored."

abellotti · 2018-02-01T16:17:15Z

app/controllers/api/service_dialogs_controller.rb

@@ -30,8 +30,9 @@ def create_resource(_type, _id, data)
    def edit_resource(type, id, data)
      service_dialog = resource_search(id, type, Dialog)
      begin
-        service_dialog.update_tabs(data['content']['dialog_tabs']) if data['content']
-        service_dialog.update!(data.except('content'))
+        api_log_warn("Both 'dialog_tabs':[...] and 'content':{'dialog_tabs':[...]} were specified. 'content':{'dialog_tabs':[...]} will be ignored.")


$api_log.warn

JPrause · 2018-02-02T14:13:19Z

As this PR is for a blocker issue, is this close to being merged?

abellotti · 2018-02-02T15:55:03Z

PR causes travis failure, code needs to be updated as noted above calling the log warn method.

miq-bot · 2018-02-03T11:02:25Z

Checked commits paulslaby/manageiq-api@906258b~...69ec56b with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

JPrause · 2018-02-05T18:01:04Z

@abellotti looks like it may be ready for merge. Fingers crossed.

jntullo

LGTM 👍

abellotti · 2018-02-06T19:14:38Z

app/controllers/api/service_dialogs_controller.rb

@@ -30,8 +30,9 @@ def create_resource(_type, _id, data)
    def edit_resource(type, id, data)
      service_dialog = resource_search(id, type, Dialog)
      begin
-        service_dialog.update_tabs(data['content']['dialog_tabs']) if data['content']
-        service_dialog.update!(data.except('content'))
+        $api_log.warn("Both 'dialog_tabs':[...] and 'content':{'dialog_tabs':[...]} were specified. 'content':{'dialog_tabs':[...]} will be ignored.")


aren't we missing if data['dialog_tags'] && data['content'] on that $api_log.warn line ?

/cc @jntullo

It just leaves out the data part, which I don't think it necessary? It still specifies the 'dialog_tabs' and 'content' part

@abellotti oh, I see what you mean. Good catch. Yes @paulslaby, please add in a condition for this line. Then it should be good to go.

Union edit service_dialog API call with other calls (cherry picked from commit f304c85) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1543119

simaishi · 2018-02-07T18:53:49Z

Gaprindashvili backport details:

$ git log -1
commit f3a5403842990ce68bbe14a4d9780136096cfe98
Author: Jillian Tullo <jntullo@bu.edu>
Date:   Wed Feb 7 11:09:33 2018 -0500

    Merge pull request #285 from paulslaby/master
    
    Union edit service_dialog API call with other calls
    (cherry picked from commit f304c851f63c2ef18cbe4ade72d4d1452eba4952)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1543119

simaishi · 2018-02-07T21:35:58Z

@paulslaby @jntullo after all, it looks we do need a separate PR for Fine branch. Both files changed in this PR are conflicting on cherry-pick as it depends on other changes not in Fine branch.

Can either one of you create a PR for Fine branch? And it's probably easier to include the follow up PR #314 as well.

jntullo · 2018-02-08T12:28:12Z

@simaishi I will work on getting that in today

paulslaby · 2018-02-08T16:46:06Z

@jntullo You were quicker than me, thank you very much. And all other involved people too 👍

JPrause · 2018-02-08T16:58:45Z

and thanks for your help as well @paulslaby

simaishi · 2018-02-08T19:03:40Z

Backported to Fine via ManageIQ/manageiq#16971

paulslaby changed the title ~~Union edit service_dialog API call with other calls~~ WIP: Union edit service_dialog API call with other calls Jan 16, 2018

paulslaby changed the title ~~WIP: Union edit service_dialog API call with other calls~~ [WIP] Union edit service_dialog API call with other calls Jan 16, 2018

miq-bot added the wip label Jan 16, 2018

simaishi added fine/yes labels Jan 16, 2018

simaishi mentioned this pull request Jan 16, 2018

[FINE] Union edit service_dialog API call with other calls - removal of cont… ManageIQ/manageiq#16747

Closed

simaishi requested a review from abellotti January 16, 2018 14:42

abellotti reviewed Jan 16, 2018

View reviewed changes

paulslaby force-pushed the master branch 2 times, most recently from 7827127 to 6bb6aa5 Compare January 16, 2018 15:55

Union edit service_dialog API call with other calls

906258b

paulslaby force-pushed the master branch from 6bb6aa5 to 906258b Compare January 16, 2018 16:32

paulslaby changed the title ~~[WIP] Union edit service_dialog API call with other calls~~ Union edit service_dialog API call with other calls Jan 18, 2018

miq-bot removed the wip label Jan 18, 2018

paulslaby force-pushed the master branch from e3d1fa9 to 8519a9c Compare January 29, 2018 08:56

abellotti reviewed Feb 1, 2018

View reviewed changes

simaishi added the blocker label Feb 2, 2018

Add warning about wrong API key used

69ec56b

paulslaby force-pushed the master branch from 8519a9c to 69ec56b Compare February 3, 2018 10:56

jntullo approved these changes Feb 5, 2018

View reviewed changes

abellotti reviewed Feb 6, 2018

View reviewed changes

jntullo merged commit f304c85 into ManageIQ:master Feb 7, 2018

jntullo mentioned this pull request Feb 7, 2018

Add condition on log warning for service dialogs #314

Merged

simaishi pushed a commit that referenced this pull request Feb 7, 2018

Merge pull request #285 from paulslaby/master

f3a5403

Union edit service_dialog API call with other calls (cherry picked from commit f304c85) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1543119

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Feb 7, 2018

simaishi added the fine/conflict label Feb 7, 2018

jntullo mentioned this pull request Feb 8, 2018

[FINE] Union edit service_dialog API call ManageIQ/manageiq#16971

Merged

simaishi added fine/backported and removed fine/conflict labels Feb 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Union edit service_dialog API call with other calls #285

Union edit service_dialog API call with other calls #285

paulslaby commented Jan 16, 2018 •

edited by simaishi

Loading

paulslaby commented Jan 16, 2018

simaishi commented Jan 16, 2018

abellotti Jan 16, 2018

paulslaby Jan 16, 2018

abellotti Jan 23, 2018

abellotti Feb 1, 2018

JPrause commented Feb 2, 2018

abellotti commented Feb 2, 2018

miq-bot commented Feb 3, 2018

JPrause commented Feb 5, 2018

jntullo left a comment

abellotti Feb 6, 2018 •

edited

Loading

jntullo Feb 6, 2018

jntullo Feb 6, 2018

simaishi commented Feb 7, 2018

simaishi commented Feb 7, 2018 •

edited

Loading

jntullo commented Feb 8, 2018

paulslaby commented Feb 8, 2018

JPrause commented Feb 8, 2018

simaishi commented Feb 8, 2018

Union edit service_dialog API call with other calls #285

Union edit service_dialog API call with other calls #285

Conversation

paulslaby commented Jan 16, 2018 • edited by simaishi Loading

paulslaby commented Jan 16, 2018

simaishi commented Jan 16, 2018

abellotti Jan 16, 2018

Choose a reason for hiding this comment

paulslaby Jan 16, 2018

Choose a reason for hiding this comment

abellotti Jan 23, 2018

Choose a reason for hiding this comment

abellotti Feb 1, 2018

Choose a reason for hiding this comment

JPrause commented Feb 2, 2018

abellotti commented Feb 2, 2018

miq-bot commented Feb 3, 2018

JPrause commented Feb 5, 2018

jntullo left a comment

Choose a reason for hiding this comment

abellotti Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

jntullo Feb 6, 2018

Choose a reason for hiding this comment

jntullo Feb 6, 2018

Choose a reason for hiding this comment

simaishi commented Feb 7, 2018

simaishi commented Feb 7, 2018 • edited Loading

jntullo commented Feb 8, 2018

paulslaby commented Feb 8, 2018

JPrause commented Feb 8, 2018

simaishi commented Feb 8, 2018

paulslaby commented Jan 16, 2018 •

edited by simaishi

Loading

abellotti Feb 6, 2018 •

edited

Loading

simaishi commented Feb 7, 2018 •

edited

Loading