-
Notifications
You must be signed in to change notification settings - Fork 143
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
Send back full dialog on create and edit #32
Conversation
@@ -18,7 +18,8 @@ def fetch_service_dialogs_content(resource) | |||
end | |||
|
|||
def create_resource(_type, _id, data) | |||
DialogImportService.new.import(data) | |||
dialog = DialogImportService.new.import(data) | |||
fetch_service_dialogs_content(dialog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we want to return both the resource + resource.content (i.e. as if we're specifying additional_attributes of content ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abellotti it does return both, there are tests for that as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still a bit confused here, the fetch__ are getters for virtual attributes. Why isn't the hook on line 3 getting this for us. set_additional_attributes asks for content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abellotti fetch_
are getters for virtual attributes - and it works as expected on :index
and :show
, as specified on line 3. This, however, is for create
and update
. I played around with this quite a bit today, and unfortunately applying the logic of line 3 to create
and update
with additional attributes will require a bit of refactoring in renderer and virtual attribute selection. i'd be happy to go forward with that work, but want to get your thoughts on whether that would be necessary.
@@ -31,7 +32,7 @@ def edit_resource(type, id, data) | |||
rescue => err | |||
raise BadRequestError, "Failed to update service dialog - #{err}" | |||
end | |||
service_dialog | |||
fetch_service_dialogs_content(service_dialog).first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there a .first here and not above on line 22 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abellotti it returns an array, and it needs to return the first element which is both the service dialog and the content
This pull request is not mergeable. Please rebase and repush. |
Checked commit jntullo@e499934 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
Thanks @jntullo for the API enhancement. LGTM!! |
When the dialog is created or edited, the entire contents of the dialog need to be returned.
cc: @romanblanco
@miq-bot add_label enhancement
@miq-bot assign @abellotti