comment id unique #5531

CleverFool77 · 2019-04-19T05:30:50Z

Fixes #4771 (<=== Add issue number here)

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/reviewers for help, in a comment below

Thanks!

plotsbot · 2019-04-19T05:42:56Z

	2 Messages
📖	@CleverFool77 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.
📖	This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: `Fixes #123`.

Generated by 🚫 Danger

jywarren · 2019-04-19T15:26:49Z

app/views/comments/_form.html.erb

@@ -1,5 +1,5 @@
 <div class="comment-form-wrapper" style="background-color:#f8f8f8; border: 1px solid #e7e7e7;padding: 18px;">
-<form class="comment-form" id="comment-form" data-remote="true" class="well" <% if !local_assigns[:is_answer].blank? %> action= "/answers/create/<%= @node.nid %>" <% elsif !local_assigns[:aid].blank? %> action= "/comment/answer_create/<%= aid %>" <% else %> action="/comment/create/<%= @node.nid %><%= "?type=question" if local_assigns[:type]=="question" %>" <% end %> <% if local_assigns[:aid].blank? %>method="post"<% end %>>
+<form class="comment-form" id="comment-form-<%= Time.now.to_s.gsub(" ", "") %><%= rand(10000) %>" data-remote="true" class="well" <% if !local_assigns[:is_answer].blank? %> action= "/answers/create/<%= @node.nid %>" <% elsif !local_assigns[:aid].blank? %> action= "/comment/answer_create/<%= aid %>" <% else %> action="/comment/create/<%= @node.nid %><%= "?type=question" if local_assigns[:type]=="question" %>" <% end %> <% if local_assigns[:aid].blank? %>method="post"<% end %>>


Hi @CleverFool77 !!! This is great, you have just the right idea. However, I think two possibilities may be good; one, to not change the id, but to use a new class (in case some other function depends on the old format of id), AND to consider using the comment cid if it exists. Here, are we able to use aid? @ViditChitkara what do you think?

Thank you!!!

Yes, I think we need to check if some other js function(drag drop or editor) depends on the comment IDs.

I think the cid might not exist for a new comment form. Maybe that's the reason why cid has not been considered? @CleverFool77

Also, the tests for the reply to comment features are not yet written so we may need to manually test if the reply to comment feature is not affected, image upload is working, propose title, etc. which appear at the bottom of the comment textbox are working.
Will be writing the tests asap. @CleverFool77 if you want, we'd love your help in writing tests.

Hi @jywarren I went through all the js functions for comments. And I found that there is no use of comment-form id. Instead mostly places in js have used classes.
Regarding other ids, I will look into it.
And as @ViditChitkara mentioned. Earlier, I was considering to use cid first but then as I went though the code base I realised the comment-form doesn't have cid. And That's why I went for current method.
And Sure, It would be great to help in writing test for comment section.

jywarren · 2019-05-08T00:32:07Z

Hi! Just checking if this is ready to merge? Thanks!!

CleverFool77 · 2019-05-08T15:26:31Z

Hi @jywarren Apologies for being pretty inactive these days.
I had exams and they extended a span of time.
Now that They have finally ended today. So I'll be back to work.
I'll be updating all my PRs and resolving conflicts soon.
Thanks for the patience.

jywarren · 2019-05-08T15:33:46Z

No problem at all, and thanks!!!!

…

On Wed, May 8, 2019, 11:27 AM Lekhika Dugtal ***@***.***> wrote: Hi @jywarren <https://github.com/jywarren> Apologies for being pretty inactive these days. I had exams and they extended a span of time. Now that They have finally ended today. So I'll be back to work. I'll be updating all my PRs and resolving conflicts soon. Thanks for the patience. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5531 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JZY23L5OLRNW2PEVMTPULWKPANCNFSM4HHCBTZQ> .

CleverFool77 · 2019-05-08T16:18:23Z

The Pr has been updated and conflict has been resolved.
Thanks !!

jywarren · 2019-05-08T17:10:11Z

Hi, @CleverFool77 -- thanks, this is awesome! I wanted to know if you'd be willing to try to write a simple system test to demonstrate the issue this solves. There are resources and some completed example tests in #5316

One way to do this is to "record" the test using https://github.com/polarblau/capycorder/ in your browser. It works /ok/. That will help protect this critical system, which keeps breaking (thank you so much for fixing it!)

CleverFool77 · 2019-09-04T13:13:38Z

Hi @jywarren I went through some old PRs. and tried working on left behind PRs. In this one, you mean to write system test. Should I write the system test of recording using capycorder you said ? or is this possible to write in screenshot test too. Can we press inspect element in screenshot test too ?

jywarren · 2019-09-04T21:23:01Z

Yes, we can add click events and then screenshot them! I'd love help with this!

…

On Wed, Sep 4, 2019 at 9:55 AM Lekhika Dugtal ***@***.***> wrote: @CleverFool77 <https://github.com/CleverFool77> requested your review on: #5531 <#5531> comment id unique. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#5531?email_source=notifications&email_token=AAAF6J553WX5LECVR6UQ6ETQH64ZLA5CNFSM4HHCBTZ2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTNSTHOI#event-2607100857>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JZGDCIQWVFIG25T4PLQH64ZLANCNFSM4HHCBTZQ> .

jywarren · 2019-12-13T22:26:43Z

Hi @CleverFool77 this bug came up again and I was wondering if we should resolve the conflicts here and test it out, and see if we can get someone else to add a system test? Thanks!

SidharthBansal · 2020-01-17T05:52:10Z

@Uzay-G @VladimirMikulic again there are some tests which you can write.

SidharthBansal · 2020-01-17T05:53:36Z

It is not necessary to get everything done in GCI. You can continue to work on issues and prs after GCI. However you will not get points for them(after GCI phase ends). I will request students to be in touch with the organizations so that studends can continue to learn. Points matter far less than learning. I hope you all want more learning after GCI too.

SidharthBansal · 2020-01-17T05:54:44Z

Students will get points till GCI ends. After GCI no points but infinite learning

SidharthBansal · 2020-01-17T05:55:07Z

@CleverFool77 kindly rebase and resolve conflicts

SidharthBansal · 2020-04-21T13:42:03Z

As the person is inactive for more than a month, I am closing the PR. In case you want to push changes please feel free to open a new PR OR reopen this PR and add additional changes to it.
Thanks for contributing on Public Lab

CleverFool77 added the in progress label Apr 19, 2019

CleverFool77 force-pushed the commnet-id branch 2 times, most recently from 0fea52f to 61b07ec Compare April 19, 2019 07:46

CleverFool77 added review-me and removed in progress labels Apr 19, 2019

jywarren reviewed Apr 19, 2019

View reviewed changes

CleverFool77 force-pushed the commnet-id branch 3 times, most recently from 29ddf05 to 5f62c60 Compare April 19, 2019 17:19

make comment id unique

9bb9585

CleverFool77 force-pushed the commnet-id branch from 5f62c60 to 9bb9585 Compare May 8, 2019 16:17

jywarren approved these changes May 8, 2019

View reviewed changes

CleverFool77 requested a review from jywarren September 4, 2019 13:54

jywarren mentioned this pull request Jan 8, 2020

Fix preview button text to go back editing #7140

Closed

SidharthBansal closed this Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

comment id unique #5531

comment id unique #5531

CleverFool77 commented Apr 19, 2019

plotsbot commented Apr 19, 2019 •

edited

jywarren Apr 19, 2019

ViditChitkara Apr 19, 2019

ViditChitkara Apr 19, 2019

ViditChitkara Apr 19, 2019

CleverFool77 Apr 19, 2019 •

edited

jywarren commented May 8, 2019

CleverFool77 commented May 8, 2019

jywarren commented May 8, 2019 via email

CleverFool77 commented May 8, 2019

jywarren commented May 8, 2019

CleverFool77 commented Sep 4, 2019 •

edited

jywarren commented Sep 4, 2019 via email

jywarren commented Dec 13, 2019

SidharthBansal commented Jan 17, 2020

SidharthBansal commented Jan 17, 2020 •

edited

SidharthBansal commented Jan 17, 2020

SidharthBansal commented Jan 17, 2020

SidharthBansal commented Apr 21, 2020

comment id unique #5531

comment id unique #5531

Conversation

CleverFool77 commented Apr 19, 2019

plotsbot commented Apr 19, 2019 • edited

jywarren Apr 19, 2019

Choose a reason for hiding this comment

ViditChitkara Apr 19, 2019

Choose a reason for hiding this comment

ViditChitkara Apr 19, 2019

Choose a reason for hiding this comment

ViditChitkara Apr 19, 2019

Choose a reason for hiding this comment

CleverFool77 Apr 19, 2019 • edited

Choose a reason for hiding this comment

jywarren commented May 8, 2019

CleverFool77 commented May 8, 2019

jywarren commented May 8, 2019 via email

CleverFool77 commented May 8, 2019

jywarren commented May 8, 2019

CleverFool77 commented Sep 4, 2019 • edited

jywarren commented Sep 4, 2019 via email

jywarren commented Dec 13, 2019

SidharthBansal commented Jan 17, 2020

SidharthBansal commented Jan 17, 2020 • edited

SidharthBansal commented Jan 17, 2020

SidharthBansal commented Jan 17, 2020

SidharthBansal commented Apr 21, 2020

plotsbot commented Apr 19, 2019 •

edited

CleverFool77 Apr 19, 2019 •

edited

CleverFool77 commented Sep 4, 2019 •

edited

SidharthBansal commented Jan 17, 2020 •

edited