-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
4635: Comment is missing from purchase details #4642
4635: Comment is missing from purchase details #4642
Conversation
91e02dc
to
1731bfe
Compare
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.
Looks great from the functional pov! Asking @dorner to take a look for any technical comments.
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.
Looking good, with some nitpicks.
app/models/purchase.rb
Outdated
@@ -69,6 +69,10 @@ def purchased_from_view | |||
vendor.nil? ? purchased_from : vendor.business_name | |||
end | |||
|
|||
def comment_view | |||
comment.nil? ? "" : comment | |||
end |
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.
I don't think this deserves a full method of its own. You can replace with purchase.comment || ''
.
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.
Made the changes here 803a9dd
<tr> | ||
<td><%= @purchase.comment_view %></td> | ||
</tr> | ||
</table> |
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.
Can you fix up the spacing here? Nested elements like tr
and td
should be indented more than the parent.
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.
I fixed here 834f25f
I also fixed the indentation of the tr
and td
tags from the table above
let!(:purchase) { create(:purchase, :with_items, item: item, storage_location: storage_location, comment: 'Fine day for diapers, it is.') } | ||
|
||
it "shows the purchase info" do | ||
escaped_html_comment = CGI.escapeHTML(purchase.comment_view) |
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.
You know exactly what the comment is - please use that value in your expectation instead of referencing the purchase
record.
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.
Made the changes here 83ee754
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.
One last suggestion!
let!(:purchase) { create(:purchase, :with_items, comment: 'Fine day for diapers, it is.', created_at: 1.month.ago, issued_at: 1.day.ago, item: item, storage_location: storage_location, vendor: vendor) } | ||
|
||
it "shows the purchase info" do | ||
date_of_purchase = "#{1.day.ago.to_fs(:distribution_date)} (entered: #{1.month.ago.to_fs(:distribution_date)})" |
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.
I think you might have to use the freeze_time
helper here, and use let
instead of let!
so that the correct dates get set. Otherwise you might get flakiness here with times being off by part of a second.
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.
I fixed here 1364f82
b7cdc30
to
a978b4b
Compare
a978b4b
to
1364f82
Compare
All good here! |
* chore(model/purchase): add comment_view * feat(view/purchases/show): show comment on purchase details * chore(model/purchase): remove comment_view * fix(purchases/show): indent html * chore(purchases_requests_rspec): use expected value on test * fix(purchases_requests_rspec): use freeze_time on show purchase info test
@victorhwmn: Your PR |
Resolves 4635
Description
I added a new table containing the comment above the list of items on view/purchases/show.html.erb, keeping it in the same div as the other purchase information. I also added a new method in the Purchase model called comment_view to avoid potential issues with a nil comment.
I then created tests for both changes. In spec/requests/purchases_requests_spec.rb, I added storage_location and vendor to set the values of the parameters used in the test. I also needed to add the constant escaped_html_comment because response.body returns escaped HTML, while purchase.comment does not.
Type of change
How Has This Been Tested?
Follow the same steps described in the issue:
The comment should appear there
Also I use the rails console to set the Purchase's comment as nil and them repeated the same steps to verify if the page would handle a null comment without breaking
Purchase.find(PURCHASE_ID).update(comment: nil)
Screenshots
Before
After
With comment:
Without comment: