Slava's Takeaway Challenge #2229
Conversation
There was a problem hiding this comment.
This is an excellent effort, well done!
I'm impressed that you have managed to mock the twilio API, and have left some suggested code which would simplify that test.
Aside from that, there are some minor suggested improvements, which don't detract from the fact that this is an excellent effort, and one you should be proud of.
| from: ENV['TWILIO_NUMBER'] | ||
| ) | ||
| puts message.sid | ||
| rescue |
There was a problem hiding this comment.
This empty rescue block just swallows any errors, without telling you / the user about them.
I'd encourage you to look into whether you can rescue a more specific error, and at least puts to share that an error occurred.
|
|
||
| it "checks if method send message was called" do | ||
| allow(subject).to receive(:text).and_return(text) | ||
| expect(text).to receive(:send_message) |
There was a problem hiding this comment.
This is good use of mocking, well done!
| body: "Thank you! Your order was placed and will be delivered before 20:00", | ||
| to: ENV['CUST_NUMBER'], | ||
| from: ENV['TWILIO_NUMBER'] | ||
| ).and_return(true) |
There was a problem hiding this comment.
You've required twilio_mock for these tests, but you're not really using it to check what's sent - instead you're mocking Twilio yourself.
Twilio mock isn't especially well documented - I ended up looking at its' own tests to understand how to use it to see the message sent: https://github.com/MaicolBen/twilio_mock/blob/master/spec/mocker_spec.rb#L48
With that, this test becomes much simpler:
it 'creates a new message' do
Timecop.freeze(Time.parse("19:00")) do
text.send_message
message = TwilioMock::Mocker.new.messages.last
expect(message.body).to eq "Thank you! Your order was placed and will be delivered before 20:00"
expect(message.to).to eq ENV['CUST_NUMBER']
expect(message.from).to eq ENV['TWILIO_NUMBER']
end
end
| it "removes the dish from an array and leaves other items in the basket" do | ||
| subject.select_dishes(1) | ||
| subject.select_dishes(5) | ||
| expect(subject.delete_dishes(1)).to eq ([{name: "Once in a lifetime Pizza", price: 9.99}]) |
There was a problem hiding this comment.
I'm not a big fan of how you're testing delete_dishes here - it's not obvious to me that returning the basket is a core part of delete_dishes's functionality. This is largely my own opinion - but I think it would be better to add 2 dishes here, delete one, and have your expect around something explicitly intended to return the basket - like subject.basket.
| require_relative 'text' | ||
|
|
||
| class Order | ||
| attr_reader :basket, :menu, :dishes, :complete, :text |
There was a problem hiding this comment.
I don't think all of these attribute readers are needed. Extra available state from inside our objects can cause problems sometimes - especially if someone else is going to use our code as an API (like a gem) - if we allow them to use more parts of our code, they are likely to rely upon them, and we have to work to maintain that too.
Yaroslava Yates
User stories
Please list which user stories you've implemented (delete the ones that don't apply).
README checklist
Does your README contains instructions for
Here is a pill that can help you write a great README!