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.

Sign up for GitHub

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

Jump to bottom

Fix raising validation if followable already exist(race condition) #5826

Merged
merged 2 commits into from
Feb 5, 2020
Merged

Fix raising validation if followable already exist(race condition) #5826

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc85b7b
Fix raising validation if followable already exist(race condition)
gonzalo-ricco-lenio Jan 31, 2020
38f31c5
Change Rails Logger for DataDogStatsClient and remove 'begin' keyword…
gonzalo-ricco-lenio Feb 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/controllers/follows_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ def follow_params
def follow(followable, need_notification: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

def is an implicit begin, so this could be:

def follow(followable, need_notification: false)
  user_follow = current_user.follow(followable)
rescue ActiveRecord::RecordInvalid => e
  Rails.logger.error(e)
end

See e.g. https://www.rubyguides.com/2019/06/ruby-rescue-exceptions/

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @citizen428 . I am seeing that codeclimate/diff-coverage check failed. How can I do to turn this check in green?

Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately the report is not very forthcoming on details, so I think you can ignore it :)

Copy link
Author

@gonzar11 gonzar11 Feb 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@citizen428 If I don't use begin then I have to use return in ensure block. Is this ok for you?

def follow(followable, need_notification: false)
  user_follow = current_user.follow(followable)
rescue ActiveRecord::RecordInvalid => e
  DataDogStatsClient.increment("users.invalid_follow")
ensure
   Notification.send_new_follower_notification(user_follow) if need_notification
   return "followed"
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I would set it up like this and actually I think that reads pretty cleanly personally. We will see what @citizen428 thinks.

def follow(followable, need_notification: false)
  user_follow = current_user.follow(followable)
  Notification.send_new_follower_notification(user_follow) if need_notification
rescue ActiveRecord::RecordInvalid => e
  DataDogStatsClient.increment("users.invalid_follow")
ensure
   return "followed"
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should really return "followed" in case something went wrong. Maybe just have different returns based on whether we succeed or fail?

def follow(followable, need_notification: false)
  user_follow = current_user.follow(followable)
  Notification.send_new_follower_notification(user_follow) if need_notification
  "followed"
rescue ActiveRecord::RecordInvalid => e
  DataDogStatsClient.increment("users.invalid_follow")
  # some other message, e.g. "previously followed"
end

Either way, it's such a small detail I'll leave it to your judgement what you think is best, i.e. if you want to keep as-is, I have no problem with that.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you folks for your help. I ended up removing ensure because Rubocop doesn't like to have a return within ensure.

user_follow = current_user.follow(followable)
Notification.send_new_follower_notification(user_follow) if need_notification
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add this to our happy path/begin block since we know if the follow is invalid we dont need to send a notification.


"followed"
rescue ActiveRecord::RecordInvalid
DataDogStatsClient.increment("users.invalid_follow")
"already followed"
end

def unfollow(followable, need_notification: false)
Expand Down