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

Steven Spiegl #2230

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@

# Local cache of Rubocop remote config
.rubocop-*
.env
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ end

group :development, :test do
gem 'rubocop', '1.20'
gem 'twilio-ruby'
gem 'dotenv'

end
38 changes: 38 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,40 @@ GEM
ast (2.4.2)
diff-lcs (1.4.4)
docile (1.4.0)
dotenv (2.7.6)
faraday (1.10.0)
faraday-em_http (~> 1.0)
faraday-em_synchrony (~> 1.0)
faraday-excon (~> 1.1)
faraday-httpclient (~> 1.0)
faraday-multipart (~> 1.0)
faraday-net_http (~> 1.0)
faraday-net_http_persistent (~> 1.0)
faraday-patron (~> 1.0)
faraday-rack (~> 1.0)
faraday-retry (~> 1.0)
ruby2_keywords (>= 0.0.4)
faraday-em_http (1.0.0)
faraday-em_synchrony (1.0.0)
faraday-excon (1.1.0)
faraday-httpclient (1.0.1)
faraday-multipart (1.0.3)
multipart-post (>= 1.2, < 3)
faraday-net_http (1.0.1)
faraday-net_http_persistent (1.2.0)
faraday-patron (1.0.0)
faraday-rack (1.0.0)
faraday-retry (1.0.3)
jwt (2.3.0)
mini_portile2 (2.8.0)
multipart-post (2.1.1)
nokogiri (1.13.4)
mini_portile2 (~> 2.8.0)
racc (~> 1.4)
parallel (1.20.1)
parser (3.0.2.0)
ast (~> 2.4.1)
racc (1.5.1)
rainbow (3.0.0)
regexp_parser (2.1.1)
rexml (3.2.5)
Expand Down Expand Up @@ -36,6 +67,7 @@ GEM
rubocop-ast (1.11.0)
parser (>= 3.0.1.1)
ruby-progressbar (1.11.0)
ruby2_keywords (0.0.5)
simplecov (0.21.2)
docile (~> 1.1)
simplecov-html (~> 0.11)
Expand All @@ -48,16 +80,22 @@ GEM
simplecov_json_formatter (0.1.3)
terminal-table (3.0.1)
unicode-display_width (>= 1.1.1, < 3)
twilio-ruby (5.66.2)
faraday (>= 0.9, < 2.0)
jwt (>= 1.5, <= 2.5)
nokogiri (>= 1.6, < 2.0)
unicode-display_width (2.0.0)

PLATFORMS
ruby

DEPENDENCIES
dotenv
rspec
rubocop (= 1.20)
simplecov
simplecov-console
twilio-ruby

RUBY VERSION
ruby 3.0.2p107
Expand Down
53 changes: 53 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,58 @@ Takeaway Challenge

```

Student notes for coach:

Firstly, all credit for the idea of a hot smoothie goes to Jonathan Goldstein, Howard Chackowicz, and Wiretap in general.

1. I was unable to implement all the tests I wanted. You'll see that I couldn't test that calling the complete_order method instantiated
a new instance of SMS. I tried a couple of tests for this, but neither worked

2. I spent a while trying to prompt the user for input to confirm their order matched their total, as I thought this would be closer to how an app would actually work (albeit with clicking instead of typing) but despite some experiments with stubbing, I was unable to get RSpec to run without at least once prompting me for input, so I ditched this.

3. I *think* I got the stubbing to work for the twilio app. In any case, rspec runs the tests without sending a message and IRB is able to send a message to my phone.
However, when I tried to implement the last two tests (point 1), I did sometimes find that rspec would send out a text. I would be keen to understand this better, so any resources on stubbing and how to get my head around it would be much appreciated.

4. I also spent a while trying to find a good test for raising an error if an item was unavailable. As things stand, I guess I ended up
hardcoding the rspec, which isn't great if this were a real app and availability needed to be changed as the test would then fail. I couldn't work out how to use doubles to instantiate a double of an unavailable item and then use that to raise the error (assuming that is what I was
supposed to be doing). The only other thing I could think of would be to create a new class for each item for sale and then have a raise_error
if that item's 'available' method was set to false, but this seems really convoluted... I'm assuming there's a way to use doubles to simulate an unavailable item, but I didn't have any joy with this, so again a nudge in the right direction would be great.

5. Another idea for improvement would be to adjust stock levels. Ideally stock would be adjusted according to what was ordered, so that the available key would return an integer rather than a boolean, and once that integer was 0, it would raise an error. There could also be low availability warnings when stock levels fell below a certain number.

6. That's everything I can think of. If you'd like to see my diagramming for this exercise, it can be found here: https://www.notion.so/Takeaway-Challenge-April-29-2022-4d2c57cfba8a4486981a71355c5c9a1b

How to install the program
------

1. Run git clone https://github.com/S-Spiegl/takeaway-challenge-new
3. If you don't have bundler already, run the command gem install bundler
2. Run bundle-install

How to use the program

-------

1. Navigate to the parent directory and open IRB
2. Enter: './lib/order.rb'
3. Create a new order (e.g. order = Order.new)
4. View the menu (e.g. order.view_menu)
5. Add items to your order using the order's (e.g. order.add(1))
6. Check your total at any time (e.g. order.total)
7. Check your selection at any time (e.g. order.selection)
8. When you're ready to checkout, run order.checkout
9. Confirm that you're happy with your order and that it matches the total, then
run complete_order and provide your telephone number to receive a confirmation
(e.g order.complete_order(+1 23456 789 10123)
10. Enjoy!

How to test

-------

Run rspec


Instructions
-------

Expand Down Expand Up @@ -81,3 +133,4 @@ Notes on Test Coverage
------------------

You can see your [test coverage](https://github.com/makersacademy/course/blob/main/pills/test_coverage.md) when you run your tests.
test
16 changes: 16 additions & 0 deletions lib/menu.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class Menu

attr_reader :items

def initialize
@items = [{ item_number: 1, scaldy: "hot pea", price: 4.00, available: true },
{ item_number: 2, scaldy: "hot tomato", price: 4.25, available: true },
{ item_number: 3, scaldy: "matzo ball", price: 5.00, available: false },
{ item_number: 4, scaldy: "hot potato", price: 3.75, available: true}
]
end

def view_menu
@items
end
end
51 changes: 51 additions & 0 deletions lib/order.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
require_relative 'menu'
require_relative 'SMS'

class Order

attr_reader :selection, :items, :total

def initialize
@selection = []
@items = Menu.new.items
@total = 0
end

def view_menu
Copy link
Author

Choose a reason for hiding this comment

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

Have implemented suggestion to move this to initialize, but have a question:

Which is better? The implemented initialize or the one below?

def initialize(items = Menu.new)
   @selection = []
   @items = items.items
   @total = 0
end

Both seem to get the same results... The first reads more clearly but makes for a longer argument

Menu.new

Choose a reason for hiding this comment

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

You previously grab the items on line 10, so you could re-use that instance of Menu to print the dishes instead.

end

def add(item_index)

fail 'item not available' if @items[item_index - 1][:available] == false
@selection << @items[item_index - 1]
@total += @items[item_index - 1][:price]
@selected_item = @items[item_index - 1][:scaldy]
item_added_confirmation
end

def checkout
p check_order_prompt, @selection, total_summary
# checkout_confirmation
end

def check_order_prompt
"Please check your order against your total:"
end

Copy link
Author

Choose a reason for hiding this comment

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

did I understand User Story 3 correctly or did I go overboard?

As a customer
So that I can verify that my order is correct
I would like to check that the total I have been given matches the sum of the various dishes in my order'

did I satisfy this condition simply by giving the customer a 'total' method?

def selection_summary
@selection
end

def total_summary
"Your total is: £#{@total}. Please use the complete_order function, entering your phone number as an argument, to complete your order"
end

def item_added_confirmation

Choose a reason for hiding this comment

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

This functionality is not mentioned in the spec. You should try to adhere to the spec as closely as possible so you're not introducing unnecessary complexity to your project.

"You successfully added #{@selected_item} to your basket"
end

def complete_order(phone_number)
SMS.new.send_sms(phone_number)
end
end
40 changes: 40 additions & 0 deletions lib/sms.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@

require 'twilio-ruby'
require 'dotenv/load'

class SMS

ETA = (Time.now + 3600).strftime("%H:%M")

attr_reader :sent, :phone_number

def initialize
@sent = false
end

def sms

@client = Twilio::REST::Client.new ENV['TWILIO_ACCOUNT_SID'], ENV['TWILIO_AUTH_TOKEN']
message = @client.messages.create(
body: "Thank you! Your order has been logged and will be with before #{ETA}",
to: "+#{@phone_number}",
from: ENV['TWILIO_PHONE'])

puts message.sid
end

def send_sms(phone_number)
@phone_number = phone_number
sms
@sent = true
sms_sent_confirmation
end

def sent?
@sent
end

def sms_sent_confirmation
"A confirmation message has been sent to the number you provided"
end
end
8 changes: 8 additions & 0 deletions spec/menu_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
require 'menu'
describe Menu do

it 'is an instance of menu' do
expect(subject).to be_instance_of(Menu)

end
end
63 changes: 63 additions & 0 deletions spec/order_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
require 'order'

describe Order do

describe '#add dishes' do

context 'when a customer selects an available dish' do
it 'it should change the order by adding a dish to it' do
expect { subject.add(1) }.to change(subject, :selection)
end
end

context 'when a customer selects an unavailable dish' do
it 'it should not let them pick a dish' do
expect { subject.add(3) }.to raise_error 'item not available'
end
end

context 'when a customer selects a meal' do
it 'it should add the cost of that dish to the total' do
expect { subject.add(1) }.to change(subject, :total)
end
end

context 'when a customer selects a dish' do
it 'it should change the confirmation message accordingly' do
expect { subject.add(1) }.to change(subject, :item_added_confirmation)
end
end

describe '#summary' do
context 'when a customer has finished choosing meals and clicks on checkout' do
it 'should show the customer their summary' do
subject.add(1)
expect(subject.checkout).to include(subject.selection)
end

it 'should prompt the customer to compare the total against the order summary' do
subject.add(1)
expect(subject.checkout).to include("Please check your order against your total:")
end
end
end
end

describe '#complete_order' do
context 'when a customer has checked their summary and enters complete_order' do
it 'should call the SMS class' do
subject.add(1)
subject.checkout
expect(subject).to respond_to(subject.complete_order).with(SMS.new.send_sms(ENV['MY_PHONE']))
end
end

Copy link
Author

@S-Spiegl S-Spiegl May 11, 2022

Choose a reason for hiding this comment

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

how to actually check that complete_order((ENV['MY_PHONE'])) calls SMS.new.send_sms? I realised I was misusing respond_to above and below comment so changed it above, but now all the test does is check that the method takes one argument. I want to implement a test to make sure that when someone enters their phone number it calls the SMS class...

context 'when a customer has checked their summary and enters complete_order' do
it 'should be instance of SMS' do
subject.add(1)
subject.checkout
expect(subject.complete_order).to_(SMS.new.send_sms)
end
end
end
end
30 changes: 30 additions & 0 deletions spec/sms_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
require 'SMS'
# require 'dotenv/load'

describe SMS do

subject(:sms) { described_class.new }

before do
allow(sms).to receive(:sms)
end

it 'sends a payment confirmation text message' do
expect(sms).to receive(:sms)
sms.send_sms(ENV['MY_PHONE'])
end

describe '#send_sms' do
it "takes a phone number as an argument" do
expect(subject).to respond_to(:send_sms).with(1).argument
end
end

it 'changes the status of sent? to true when a message is sent' do
expect { subject.send_sms(ENV['MY_PHONE']) }.to change(subject, :sent?).to true
end

it 'confirms a message has been sent' do
expect(subject.sms_sent_confirmation).to eq('A confirmation message has been sent to the number you provided')
end
end