-
Notifications
You must be signed in to change notification settings - Fork 2k
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
base: main
Are you sure you want to change the base?
Steven Spiegl #2230
Changes from 1 commit
49a36bf
d418efd
ecc2598
fbf1603
c884e34
24da351
1fba4bf
d4f5797
b2e345c
1cdf619
b6b7fac
7c626af
afb5210
f53cf28
2df5680
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,4 +13,5 @@ def initialize | |
def view_menu | ||
@items | ||
end | ||
#is this method necessary? Taking it out doesn't break tests... | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,18 +5,18 @@ class Order | |
|
||
attr_reader :selection, :items, :total | ||
|
||
def initialize | ||
def initialize(items = Menu.new.items) | ||
@selection = [] | ||
@items = Menu.new.items | ||
@items = items | ||
@total = 0 | ||
end | ||
|
||
def view_menu | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Both seem to get the same results... The first reads more clearly but makes for a longer argument |
||
Menu.new | ||
@items | ||
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] | ||
|
@@ -33,6 +33,13 @@ def check_order_prompt | |
"Please check your order against your total:" | ||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did I understand User Story 3 correctly or did I go overboard?
did I satisfy this condition simply by giving the customer a 'total' method? |
||
#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 | ||
|
@@ -49,3 +56,6 @@ def complete_order(phone_number) | |
SMS.new.send_sms(phone_number) | ||
end | ||
end | ||
|
||
#complete_order is failing its tests, saying that it expected 1 argument | ||
#and got 0, even |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,15 +48,18 @@ | |
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'])) | ||
expect(subject).to respond_to(:complete_order).with(1).argument | ||
end | ||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... |
||
#how to actually check that the complete_order((ENV['MY_PHONE'])) calls | ||
# SMS.new.send_sms? | ||
|
||
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) | ||
expect(subject.complete_order).to respond_to(SMS.new.send_sms) | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,3 @@ | ||
require 'SMS' | ||
# require 'dotenv/load' | ||
|
||
describe SMS do | ||
|
||
subject(:sms) { described_class.new } | ||
|
@@ -19,7 +16,7 @@ | |
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 | ||
|
@@ -28,3 +25,6 @@ | |
expect(subject.sms_sent_confirmation).to eq('A confirmation message has been sent to the number you provided') | ||
end | ||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do these tests actually check an sms has been sent, or just that the class acts as though a message has been sent (e.g. changes sent_confirmation) |
||
#do these tests actually check an sms has been sent, or just that | ||
#the class acts as though a message has been sent (e.g. changes sent_confirmation) |
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.
is this method necessary? Taking it out doesn't break tests...