Skip to content

Fix deal creation with hash as argument#39

Merged
piotr-szczesniak merged 1 commit into
zendesk:masterfrom
piotrj:fix_deal_create
Nov 18, 2016
Merged

Fix deal creation with hash as argument#39
piotr-szczesniak merged 1 commit into
zendesk:masterfrom
piotrj:fix_deal_create

Conversation

@piotrj
Copy link
Copy Markdown
Contributor

@piotrj piotrj commented Jul 29, 2016

Fix a bug introduced in 78112f2 where it was impossible to create a deal
by just passing a hash to create. sanitize method was expecting
passed argument to respond to value which in case of hash was failing.

Fix a bug introduced in 78112f2 where it was impossible to create a deal
by just passing a hash to `create`. `sanitize` method was expecting
passed argument to respond to value which in case of hash was failing.
it "returns instance of Deal class" do
@deal = build(:deal)
expect(client.deals.create(@deal)).to be_instance_of BaseCRM::Deal
deal = build(:deal)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't you think that this change is not significant enough to add chaos to history of this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes it is not significant.
But I just added a spec below that follows the same convention and I don't want to assign stuff to instance variable cause it does not make sense here (and adds confusion since you start wondering whether this variable is used somewhere else). So I went for local variable and changed this spec so that both of them follow the same pattern.

@mms-pl
Copy link
Copy Markdown
Contributor

mms-pl commented Jul 29, 2016

Other than this, looks got to me.

@piotr-szczesniak piotr-szczesniak merged commit 00a0763 into zendesk:master Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants