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

remove answer notification code #5496

Merged

Conversation

ViditChitkara
Copy link
Member

Part of #4094

@plotsbot
Copy link
Collaborator

plotsbot commented Apr 16, 2019

1 Warning
⚠️ This pull request cannot be merged yet due to merge conflicts. Please resolve them – probably by rebasing – and ask for help (in the comments, or in the chatroom if you get into trouble!.
1 Message
📖 @ViditChitkara Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@ViditChitkara ViditChitkara force-pushed the answer-notification-deprication branch from afc3267 to ddbea13 Compare May 4, 2019 15:09
@jywarren
Copy link
Member

jywarren commented May 7, 2019

Looks good; we resolved an upstream test failure today so perhaps this just needs a rebase? Not sure, though! Thanks!!!!

@ViditChitkara ViditChitkara force-pushed the answer-notification-deprication branch from e1ab6bc to a246f7b Compare May 31, 2019 17:29
@ViditChitkara
Copy link
Member Author

ViditChitkara commented May 31, 2019

@publiclab/reviewers , @jywarren , @jainaman224 I think this is ready to merge. Kindly have a look.

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

Looks good but is this supposed to be there? Just checking!

db/migrate/.20190401093400_update_node_view_count 2.rb.icloud

@@ -11,8 +11,7 @@ def likes
AnswerSelection.set_likes(current_user.uid, @answer.id, false)
else
AnswerSelection.set_likes(current_user.uid, @answer.id, true)
user = User.find(current_user.uid)
AnswerMailer.notify_answer_like(user, @answer).deliver_later
User.find(current_user.uid)
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 this is not required. We already have user as current_user.
😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that line didn't make any sense. Thanks for pointing out!!

@ViditChitkara
Copy link
Member Author

Looks good but is this supposed to be there? Just checking!

Woah, I think that was added by mistake. Removed it.

@ViditChitkara ViditChitkara force-pushed the answer-notification-deprication branch from b3bb9af to 402552b Compare June 1, 2019 18:05
@jywarren
Copy link
Member

jywarren commented Jun 5, 2019

Looks good!!!! Thank you!!!

@jywarren jywarren merged commit 16ca5be into publiclab:master Jun 5, 2019
enviro3 pushed a commit to enviro3/plots2 that referenced this pull request Aug 15, 2019
* remove answer notification code

* minor changes

* fix answer tests

* minor changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants