Alert for accepted answer #1051

500swapnil · 2016-12-02T10:23:30Z

Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!

all tests pass -- rake test:all
code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
pull request is descriptively named with #number reference back to original issue
if possible, multiple commits squashed if they're smaller changes
reviewed/confirmed/tested by another contributor or maintainer
schema.rb.example has been updated if any database migrations were added

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays. Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.

Thanks!

500swapnil · 2016-12-02T10:24:33Z

#847

500swapnil · 2016-12-02T10:25:52Z

What should I write instead of ? Wont the nid differ for every question?

jywarren · 2016-12-03T22:53:28Z

app/views/questions/_answer.html.erb

@@ -32,7 +32,7 @@
    <hr />
    <div style="padding: 0px 20px 10px 20px;">
    <% if current_user && current_user.uid == answer.node.uid || answer.accepted %>
-      <a <% if current_user && current_user.uid == answer.node.uid %> href="/answers/accept/<%= answer.id %>" <% end %> data-remote="true" id="answer-<%= answer.id %>-accept" class="answer-accept btn btn-sm <% if answer.accepted %>btn-success<% else %>btn-default<% end%>"><% if answer.accepted %>Accepted<% else %>Accept this answer<% end %></a>
+      <a <% if current_user && current_user.uid == answer.node.uid %> href="/answers/accept/<%= answer.id %>" <% end %> data-remote="true" id="answer-<%= answer.id %>-accept" class="answer-accept btn btn-sm <% if answer.accepted %>btn-success<% else %>btn-default<% end%>"><% if answer.accepted %><p class="alert alert-success">Your answer was accepted! If it would be helpful, consider expanding on this answer by <a href="/post?template=activity,tags=response:<question-nid>">posting a step-by-step activity</a></p><% else %>Accept this answer<% end %></a>


Hi, thanks for the PR!

Here, I think it'll be easier if you break it up into multiple lines; what's happening is we don't really want to delete the existing text, but to add the <div class="alert... onto its own line.

And I think something else is going on too -- this conditional checks if the current user is the same as the question asker -- answer.node.uid -- but we want to know if it's the same as the answerer -- that'd be, I think, answer.author.uid according to this file: https://github.com/publiclab/plots2/blob/master/app/models/answer.rb

And as to where, I think we could display the alert right above the horizontal rule (<hr />) on this line: https://github.com/500swapnil/plots2/blob/21b7823534b62e9564839f15c9e2be6356efeab3/app/views/questions/_answer.html.erb#L32

Take a look at a page with answers, does that placement make sense to you?

https://publiclab.org/questions/dhale2/12-01-2016/spectral-workbench-recording-time

Screenshot:

jywarren · 2016-12-03T22:58:17Z

Also, yes -- <question-nid> should become <%= answer.nid %> -- make sense?

500swapnil · 2016-12-05T14:45:14Z

app/views/questions/_answer.html.erb

    <hr />
    <div style="padding: 0px 20px 10px 20px;">
-    <% if current_user && current_user.uid == answer.node.uid || answer.accepted %>
-      <a <% if current_user && current_user.uid == answer.node.uid %> href="/answers/accept/<%= answer.id %>" <% end %> data-remote="true" id="answer-<%= answer.id %>-accept" class="answer-accept btn btn-sm <% if answer.accepted %>btn-success<% else %>btn-default<% end%>"><% if answer.accepted %><p class="alert alert-success">Your answer was accepted! If it would be helpful, consider expanding on this answer by <a href="/post?template=activity,tags=response:<question-nid>">posting a step-by-step activity</a></p><% else %>Accept this answer<% end %></a>
+    <% if current_user && current_user.uid == answer.author.uid || answer.accepted %>


I think this line should have answer.node.uid so that the person who asks the questions can choose to accept it.
What do you think?

500swapnil · 2016-12-05T14:46:35Z

In the alert on line 32, I have made the condition answer.author.uid so that the answerer shall be notified whether his answer is accepted. But shouldnt the condition check for answer.node.uid on line 39?

jywarren · 2016-12-05T15:36:17Z

Yes, I think you may have identified an existing bug in the display of alerts. Three conditions, really:

"Accepted" should be displayed to everyone
"Accept this answer" only to the question author.
"Your question was accepted... consider making an activity" (etc, your addition) to the answer author

Thanks for catching this! I think multiple lines will make it much easier to identify these three conditions.

jywarren · 2016-12-05T18:28:30Z

app/views/questions/_answer.html.erb

-    <% if current_user && current_user.uid == answer.author.uid || answer.accepted %>
-      <a <% if current_user && current_user.uid == answer.author.uid %> href="/answers/accept/<%= answer.id %>" <% end %> data-remote="true" id="answer-<%= answer.id %>-accept" class="answer-accept btn btn-sm <% if answer.accepted %>btn-success<% else %>btn-default<% end%>"><% if answer.accepted %>Accepted<% else %>Accept this answer<% end %></a>
+    <% if current_user && current_user.uid == answer.node.uid || answer.accepted %>
+      <a <% if current_user && current_user.uid == answer.node.uid %> href="/answers/accept/<%= answer.id %>" <% end %> data-remote="true" id="answer-<%= answer.id %>-accept" class="answer-accept btn btn-sm <% if answer.accepted %>btn-success<% else %>btn-default<% end%>"><% if answer.accepted %>Accepted<% else %><% if current_user && current_user.uid == answer.node.uid %>Accept this answer<% end %><% end %></a>


For readability (and because the first is not actually a working button, even if it uses similar styling), could you break out the Accepted and Accept this answer messages into two separate lines, and two separate <a> tags?

If "Accepted" isnt a working button, I needn't put it into "" tags right?

jywarren · 2016-12-05T19:35:01Z

I believe you can use a `<span>` instead and it'll still have the styling but won't be "clickable". Thanks!

…

On Mon, Dec 5, 2016 at 2:02 PM, 500swapnil ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/views/questions/_answer.html.erb <#1051>: > @@ -36,8 +36,8 @@ <% end %> <hr /> <div style="padding: 0px 20px 10px 20px;"> - <% if current_user && current_user.uid == answer.author.uid || answer.accepted %> - <a <% if current_user && current_user.uid == answer.author.uid %> href="/answers/accept/<%= answer.id %>" <% end %> data-remote="true" id="answer-<%= answer.id %>-accept" class="answer-accept btn btn-sm <% if answer.accepted %>btn-success<% else %>btn-default<% end%>"><% if answer.accepted %>Accepted<% else %>Accept this answer<% end %></a> + <% if current_user && current_user.uid == answer.node.uid || answer.accepted %> + <a <% if current_user && current_user.uid == answer.node.uid %> href="/answers/accept/<%= answer.id %>" <% end %> data-remote="true" id="answer-<%= answer.id %>-accept" class="answer-accept btn btn-sm <% if answer.accepted %>btn-success<% else %>btn-default<% end%>"><% if answer.accepted %>Accepted<% else %><% if current_user && current_user.uid == answer.node.uid %>Accept this answer<% end %><% end %></a> If "Accepted" isnt a working button, I needn't put it into "" tags right? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1051>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ_JeLD4_Yhx5xNDTd9H24rL8k6vUks5rFF_AgaJpZM4LCZsX> .

500swapnil · 2016-12-05T19:44:48Z

Left it at '' . I separated them though. Let me know if any other changes are needed.

ryzokuken · 2016-12-06T03:15:43Z

Hello! You must've noticed that the unit tests failed unexpectedly on your PR. This was due to some issue in the Travis CI testing configuration which was fixed by @jywarren later tonight. In order to make them work again, please rebase your work on top of the current master and then try pushing to your remote branch. Do this by:

$ git fetch upstream
$ git checkout master
$ git merge upstream/master
$ git checkout <my-branch>
$ git rebase master
$ git push origin <my-branch>

Thanks for bearing with us,
Ujjwal

500swapnil · 2016-12-06T05:29:26Z

Okay done

ryzokuken · 2016-12-06T06:29:18Z

Great work!
Looks like you haven't configured git properly on your device. That's why github's not showing the commits to be added by you. Try:

$ git config --global user.name "<Your name as on Github>"
$ git config --global user.email "<Your email as on Github>"

500swapnil · 2016-12-06T06:36:11Z

Thanks. I added my username and email. It will show up on the other changes I make.

ryzokuken · 2016-12-06T06:38:38Z

Bravo. @jywarren will look into this further. If you're done with this issue, feel free to pick another one up and ask for help if needed on our Gitter channel and/or on Github itself.

500swapnil · 2016-12-06T06:40:34Z

Thank you. Yeah I will pick another.

jywarren · 2016-12-06T12:19:15Z

Perfect and much more readable!!!

jywarren · 2016-12-06T17:03:54Z

Hi, @publiclab/reviewers - if anyone's able to take a look at this, that'd be super, thanks! I'm still working on a bug with the Features system. Thanks!

jywarren · 2016-12-06T20:09:58Z

oh weird i guess i submitted this comment out of order -- all good!

Alert for accepted answer

21b7823

500swapnil mentioned this pull request Dec 2, 2016

Show notice on accepted Answers prompting them to be adapted into Activities #847

Closed

jywarren reviewed Dec 3, 2016

View reviewed changes

Swapnil Gupta and others added 2 commits December 5, 2016 13:04

xMerge https://github.com/publiclab/plots2

852f615

Is it okay now?

fe3a8b6

500swapnil commented Dec 5, 2016

View reviewed changes

Should work fine now

54e8489

jywarren reviewed Dec 5, 2016

View reviewed changes

Update _answer.html.erb

93c5614

Merge branch 'master' of https://github.com/500swapnil/plots2

46b0fb7

Merge remote-tracking branch 'upstream/master'

214cb84

jywarren merged commit 89bb691 into publiclab:master Dec 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alert for accepted answer #1051

Alert for accepted answer #1051

500swapnil commented Dec 2, 2016

500swapnil commented Dec 2, 2016

500swapnil commented Dec 2, 2016

jywarren Dec 3, 2016

jywarren commented Dec 3, 2016

500swapnil Dec 5, 2016

500swapnil commented Dec 5, 2016

jywarren commented Dec 5, 2016

jywarren Dec 5, 2016

500swapnil Dec 5, 2016

jywarren commented Dec 5, 2016 via email

500swapnil commented Dec 5, 2016

ryzokuken commented Dec 6, 2016

500swapnil commented Dec 6, 2016

ryzokuken commented Dec 6, 2016

500swapnil commented Dec 6, 2016

ryzokuken commented Dec 6, 2016 •

edited

Loading

500swapnil commented Dec 6, 2016

jywarren commented Dec 6, 2016

jywarren commented Dec 6, 2016

jywarren commented Dec 6, 2016

Alert for accepted answer #1051

Alert for accepted answer #1051

Conversation

500swapnil commented Dec 2, 2016

500swapnil commented Dec 2, 2016

500swapnil commented Dec 2, 2016

jywarren Dec 3, 2016

Choose a reason for hiding this comment

jywarren commented Dec 3, 2016

500swapnil Dec 5, 2016

Choose a reason for hiding this comment

500swapnil commented Dec 5, 2016

jywarren commented Dec 5, 2016

jywarren Dec 5, 2016

Choose a reason for hiding this comment

500swapnil Dec 5, 2016

Choose a reason for hiding this comment

jywarren commented Dec 5, 2016 via email

500swapnil commented Dec 5, 2016

ryzokuken commented Dec 6, 2016

500swapnil commented Dec 6, 2016

ryzokuken commented Dec 6, 2016

500swapnil commented Dec 6, 2016

ryzokuken commented Dec 6, 2016 • edited Loading

500swapnil commented Dec 6, 2016

jywarren commented Dec 6, 2016

jywarren commented Dec 6, 2016

jywarren commented Dec 6, 2016

ryzokuken commented Dec 6, 2016 •

edited

Loading