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

Made changes to code to not save blank resource action #2961

Merged

Conversation

h-kataria
Copy link
Contributor

  • Dont add a blank resource action when saving catalog item.
  • Delete empty/blank resource action when user has removed entry point while editing Catalog Item. Changed code that sets entry points to nil when displaying a Catalog Item, it was displaying incorrect values even after entry point was removed.

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

@gmcculloug please review/test. If this looks good i will work on adding a spec test around this.

@h-kataria h-kataria force-pushed the dont_save_blank_resource_action branch 3 times, most recently from e08d81b to b68f819 Compare May 20, 2015 19:30
else
ra.dialog = d
ra.fqname = @edit[:new][action[:edit_key]].blank? ? nil : @edit[:new][action[:edit_key]]
ra.save!
Copy link
Member

Choose a reason for hiding this comment

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

You do not need the check for @edit[:new][action[:edit_key]] being blank since that case is handled by the if statement.
The then entire else section could be reduced to:
ra.update_attributes(:dialog => d, :fqname => @edit[:new][action[:edit_key]])

@h-kataria h-kataria force-pushed the dont_save_blank_resource_action branch from b68f819 to 27a2bbc Compare May 20, 2015 21:29
@h-kataria
Copy link
Contributor Author

@gmcculloug made suggested changes.

@h-kataria h-kataria force-pushed the dont_save_blank_resource_action branch from 27a2bbc to 3c7c259 Compare May 20, 2015 21:33
@@ -1214,7 +1214,7 @@ def trees_to_replace(trees)
end

def set_resource_action(st)
d = @edit[:new][:dialog_id].nil? ? nil : Dialog.find_by_id(@edit[:new][:dialog_id])
d = @edit[:new][:dialog_id].nil? ? nil : Dialog.where(:id => @edit[:new][:dialog_id].to_i).first
Copy link
Member

Choose a reason for hiding this comment

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

You do not need to call .to_i.

@gmcculloug
Copy link
Member

@h-kataria One additional comment and could you also take a look at the recommended spec changes.

@h-kataria
Copy link
Contributor Author

@gmcculloug i will make the suggested change, not sure which spec change you are reffering to, i had already made suggested changes to spec test from your previous suggestions

@gmcculloug
Copy link
Member

@h-kataria I was expecting my comments to be marked as outdated diffs. You do not need the length check anymore since that is covered by the match_array check.

- Dont add a blank resource action when saving catalog item.
- Delete empty/blank resource action when user has removed entry point while editing Catalog Item. Changed code that sets entry points to nil when displaying a Catalog Item, it was displaying incorrect values even after entry point was removed.
- Added spec test to verify blank resource action does not get saved.

https://bugzilla.redhat.com/show_bug.cgi?id=1221333
@h-kataria h-kataria force-pushed the dont_save_blank_resource_action branch from 3c7c259 to f194cbd Compare May 21, 2015 13:34
@h-kataria
Copy link
Contributor Author

@gmcculloug made suggested changes, please re-review.

@miq-bot
Copy link
Member

miq-bot commented May 21, 2015

Checked commit h-kataria@f194cbd with rubocop 0.27.1
2 files checked, 0 offenses detected
Everything looks good. 👍

gmcculloug added a commit that referenced this pull request May 21, 2015
Made changes to code to not save blank resource action
@gmcculloug gmcculloug merged commit 46f9663 into ManageIQ:master May 21, 2015
@chessbyte chessbyte added this to the Sprint 24 Ending June 1, 2015 milestone May 22, 2015
@h-kataria h-kataria deleted the dont_save_blank_resource_action branch June 22, 2015 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants