Skip to content

Fix/pay from offer #316

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

Merged
merged 3 commits into from
Feb 2, 2018
Merged

Fix/pay from offer #316

merged 3 commits into from
Feb 2, 2018

Conversation

sauloperez
Copy link
Collaborator

@sauloperez sauloperez commented Feb 1, 2018

What? Why?

Closes #315.

I forgot to provide the destination of the account in the link to transfer, showed on the offer page. This was fetched in UsersController#give_time and later passed to the give_time view.

When I did the refactor I forgot to move that from https://github.com/coopdevs/timeoverflow/pull/264/files#diff-4e05ad0d64e6100656b63ad1e78f32c5L67 to the appropriate place (the OffersController#show), so that we specify the destination in the link_to.

Introducing view specs 🎉

Since this had to do also with the view, I thought it was the perfect timing to start doing view specs. I've wanted to add them soon to replace the pseudo pattern matching that we ended up doing in some controller specs to ensure stuff in the views.

I figured it'd be faster to get the test I wanted for the view in this way rather than using the current approach in the controller spec. And I was right. Having the capybara RSpec matchers available, such as have_link help a lot. You spend less time checking how does the actual returned HTML look like.

Copy link
Contributor

@enricostano enricostano left a comment

Choose a reason for hiding this comment

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

👏

def show
super

if current_user
Copy link
Contributor

@enricostano enricostano Feb 1, 2018

Choose a reason for hiding this comment

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

I'd rather move this check to the view

@sseerrggii
Copy link
Contributor

Works fine 👍

@sauloperez sauloperez self-assigned this Feb 2, 2018
@sauloperez sauloperez merged commit ea0b66b into develop Feb 2, 2018
@sauloperez sauloperez deleted the fix/pay-from-offer branch February 2, 2018 15:07
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