added "Suggest new title" feature #2000

ViditChitkara · 2018-01-13T20:25:56Z

closes #919

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

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!

ViditChitkara · 2018-01-13T20:29:57Z

@jywarren I have a raw idea about this. Just like inline power tags there could be another syntax which could create the specified template for title suggestion (e.g {New title}).
Does this make sense?

PublicLabBot · 2018-01-13T20:38:37Z

	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

jywarren

This is a pretty interesting approach -- happy to help refine it and get it ready! Thanks!

jywarren · 2018-01-14T00:49:04Z

config/routes.rb

@@ -6,6 +6,7 @@
  post 'comment/create/token/:id.:format', to: 'comment#create_by_token'

  get 'searches/test' => 'searches#test'
+  post '/request_title_change/:id/:title' => 'notes#request_title_change'


Maybe we could simplify this and also not put the title itself in the path, as it may contain punctuation? It could be using ?title=____ instead.

Sure, we could do the same with id!! Also we'd need to have a check in the request_title_update method, since the route could easily be manipulated to change the id or title so an authorisation check is a must at the back-end.

jywarren · 2018-01-14T00:50:04Z

app/views/notes/_comment.html.erb

@@ -64,7 +64,7 @@
  </script>

  <div id="c<%= comment.cid %>show">
-    <p><%= raw sanitize(auto_link(RDiscount.new(comment.body).to_html)) %></p>
+    <p><%= raw sanitize (RDiscount.new(title_suggestion(comment.body, comment.drupal_user.name, comment.nid, comment.id)).to_html), attributes: %w(class style href data-method) %></p>


This isi getting quite long; any way to simplify via formatting?

Maybe pass the comment object to title_suggestion method would be better rather than breaking it up into body, name, etc. 'Attributes' require href and data-method for the time being perhaps. I'll try something to shorten this!!,

jywarren · 2018-01-16T17:45:26Z

PR #2000 -- cool! :-)

jywarren · 2018-01-16T17:46:51Z

app/views/notes/_comment.html.erb

@@ -64,7 +64,7 @@
  </script>

  <div id="c<%= comment.cid %>show">
-    <p><%= raw sanitize(auto_link(RDiscount.new(comment.body).to_html)) %></p>
+    <p><%= raw sanitize (RDiscount.new(title_suggestion(comment)).to_html), attributes: %w(class style href data-method src) %></p>


This starts to get quite long. See how we've batched these in this code?

https://github.com/ViditChitkara/plots2/blob/624aa873794f0d904fb44e0772a2fddf51309d5d/app/helpers/application_helper.rb#L30-L42

What do you think about making this more readable with an additional helper somehow, rather than a long in-line code block?

Looking good though!

Some helper method like:- comment_body
It could look like this:-

def comment body(comment) RDiscount.new(title_suggestion(comment)).to_html), attributes: %w(class style href data-method src end

Then we could use it in line 67 as:-

<p><%= raw sanitize(auto_link(RDiscount.new(comment_body(comment)).to_html)) %></p>

Will this be good?

jywarren · 2018-01-16T17:47:11Z

app/views/notes/_title_suggestion.html.erb

@@ -0,0 +1,6 @@
+<blockquote>
+	<a href="/profile/<%= user %>"><%= user %></a> is suggesting an alternative title, "<%= title %>".


Can we switch to 2-space indents here? sorry to be particular :-)

Also, can you share a screenshot of what this looks like?

Oh I missed that!! I'll do!!

Yes, I'll share the screenshots!

jywarren

This is a complex feature and I love your implementation. Made a few requests, and perhaps a single line comment like this above some of your method definitions could help make it clear how this new function works/

# explanation
def method_name
  ...
end

jywarren · 2018-01-16T17:48:48Z

config/routes.rb

@@ -6,6 +6,7 @@
  post 'comment/create/token/:id.:format', to: 'comment#create_by_token'

  get 'searches/test' => 'searches#test'
+  post '/reset-title' => 'notes#request_title_change'


Does this actually change the title, or does it request a title change? I think it actually changes the title. Maybe we could do like /node/update/title or something more descriptive? Same for the method name request_title_change -- could that be update_title to be more accurate, or do i misunderstand?

This actually changes the title. Yes, that's a cool idea!! I'll change the route name and method name to the above.

ViditChitkara · 2018-01-16T21:05:37Z

@jywarren Here are the screenshots of the feature

jywarren · 2018-01-16T21:15:01Z

aha - cool. Do you think perhaps it should be something like this, though, so we can expand out for other types of inline comment features in the future?

[propose:title:_____________]

ViditChitkara · 2018-01-16T21:30:48Z

This syntax looks cool indeed!! I'll do the changes

jywarren · 2018-01-16T21:58:17Z

Or:

[propose:title]This is a new title[/propose]

which do you think makes more sense?

closes publiclab#919

ViditChitkara · 2018-01-17T15:37:24Z

@jywarren I have made the changes!! May I write tests if the design seems to be finalised?

jywarren · 2018-01-17T16:02:39Z

Let's see -- one thing is, perhaps we can use an <div class="alert alert-warning"></div> instead of a blockquote? That'll seem more like an official feature.

Also, just to think through this, what if you press this twice? It'll just continually replace the title, right? No possibility of weird behavior I can think of... just want to be thorough. OK, let's go for tests then!

ViditChitkara · 2018-01-17T16:08:34Z

Yes, it would just replace the title continuously, I have tried that too and no weird behaviours were seen. I'd like to try and replace the block quote with something mentioned above.

ViditChitkara · 2018-01-17T16:10:26Z

Duh! That was closed by mistake! I'll write the tests!

ViditChitkara · 2018-01-17T18:28:24Z

@jywarren written the tests and also replaced the blockquotes with <div class="alert alert-warning"></div>.
This actually looks nice with the alert panel!! Thanks for the suggestion.
Could you please see if this is good to go now?

jywarren · 2018-01-17T23:17:22Z

Wow, very cool! I think we are ready to go; do you think we should try this out once merged, then think about a user interface as a follow-up? We could for example add a button like the "promote to answer" button on comments -- which could use a JavaScript prompt() to ask for the proposed title.

The button could appear as it does in this change by @Gauravano -- https://github.com/publiclab/plots2/pull/2001/files

What do you think, shall we merge this in, test it out, then think about UI as a follow-up?

ViditChitkara · 2018-01-18T04:39:12Z

This could make a really good first timer issue. Let's test this out on production and see if it works or not. Then, should I create a first-timer-issue for the follow-up part? This would be quite a task but would be very interesting for new comers!!

…

On Thu, Jan 18, 2018 at 4:47 AM, Jeffrey Warren ***@***.***> wrote: Wow, very cool! I think we are ready to go; do you think we should try this out once merged, then think about a user interface as a follow-up? We could for example add a button like the "promote to answer" button on comments -- which could use a JavaScript prompt() to ask for the proposed title. The button could appear as it does in this change by @Gauravano <https://github.com/gauravano> -- https://github.com/publiclab/ plots2/pull/2001/files What do you think, shall we merge this in, test it out, then think about UI as a follow-up? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2000 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVkX0cH5VclQr-TWFsaR-UNB_O69Hvvfks5tLn-EgaJpZM4RdW21> .

ViditChitkara · 2018-01-18T05:56:39Z

@jywarren I was just thinking of another way of doing this! We would have a button in the comments section (which would say something like 'propose title'). When clicked, it would append the template for title suggestion like [propose:title]Propose your title here[/propose] in the comment editor.
This could be useful if a user wants to propose more than one title in a single comment. Then there would be no need of the prompt.
Your thoughts?

jywarren · 2018-01-19T21:50:30Z

Oh, that sounds really nice -- and could still be a first-timers-issue -- super!

…

On Thu, Jan 18, 2018 at 12:56 AM, Vidit ***@***.***> wrote: @jywarren <https://github.com/jywarren> I was just thinking of another way of doing this! We would have a button in the comments section (which would say something like 'propose title'). When clicked, it would append the template for title suggestion like [propose:title]Propose your title here[/propose] in the comment editor. This could be useful if a user wants to propose more than one title in a single comment. Then there would be no need of the prompt. Your thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2000 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ4TVjY_-f_wfIH9eTCBNjI-XiK88ks5tLt0XgaJpZM4RdW21> .

ViditChitkara · 2018-01-20T06:26:49Z

Cool!! Do you think we should merge this? I will create a fto issue for the left over part as explained above!!
Thanks.

ViditChitkara · 2018-01-22T16:12:54Z

@jywarren please see if there are any more required changes to it.

jywarren · 2018-01-22T16:56:54Z

Hi, I guess i'd like to know if there's a way to test the conversion of the [propose:title] tags in the helper code... what do you think? if it's too complex, we can just merge this, but it'd be nice to have a test for that too, no?

Sorry to be delaying on this a bit, it's a great feature :-) 👍

ViditChitkara · 2018-01-22T19:22:12Z

That's interesting!! I'll do add the tests!!

ViditChitkara · 2018-01-24T16:18:07Z

@jywarren the tests are written!! Please have a look!

jywarren · 2018-01-25T00:58:24Z

Wow, this is so cool! 🎉

Merging this! I hope we can publish tomorrow, we'll see!

jywarren · 2018-01-26T00:12:56Z

Hi, @ViditChitkara -- just noting one follow-up issue -- i see an error:

/app/app/helpers/application_helper.rb:55: warning: regular expression has ']' without escape: /\[propose:title](.*?)\[\/propose]/
/app/app/helpers/application_helper.rb:55: warning: regular expression has ']' without escape: /\[propose:title](.*?)\[\/propose]/

Looks like maybe we need to escape that?

ViditChitkara · 2018-01-26T06:53:48Z

Oh I had this warning too, slipped out of my mind. Will give a quick fix to update this!!

namangupta01 · 2018-01-26T06:55:41Z

@ViditChitkara @jywarren working on this in #2125

ViditChitkara · 2018-01-26T07:20:19Z

Oops sorry didn't see that!! So are you covering the regex warning in fixing the robocup offences?

namangupta01 · 2018-01-26T07:33:20Z

@ViditChitkara yes i am covering all the rubocop offences there
see here: https://pastebin.com/raw/J2kxYX66

jywarren · 2018-01-26T19:02:15Z

great, thanks folks!

* added "Suggest new title" feature closes publiclab#919 * formatted routes * few changes for simplification * authorisation check to request_title_change method * changes for test faliures * minor changes and enhancements * minor changes * changed inline syntax for title suggestion * written tests * added more tests

jywarren reviewed Jan 14, 2018

View reviewed changes

ViditChitkara force-pushed the suggest-new-title-feature branch from 016d34f to 06b292b Compare January 14, 2018 14:35

jywarren reviewed Jan 16, 2018

View reviewed changes

ViditChitkara force-pushed the suggest-new-title-feature branch from 5193310 to 1e15361 Compare January 16, 2018 20:45

ViditChitkara added 8 commits January 17, 2018 20:44

added "Suggest new title" feature

9a006ac

closes publiclab#919

formatted routes

3fd2efe

few changes for simplification

545b2ca

authorisation check to request_title_change method

3ff1d39

changes for test faliures

530bec9

minor changes and enhancements

eac4062

minor changes

4ae2035

changed inline syntax for title suggestion

8876e12

ViditChitkara force-pushed the suggest-new-title-feature branch from 1e15361 to 8876e12 Compare January 17, 2018 15:14

ViditChitkara closed this Jan 17, 2018

ViditChitkara reopened this Jan 17, 2018

written tests

f23a1c7

added more tests

49187f8

jywarren merged commit 71c01bd into publiclab:master Jan 25, 2018

jywarren mentioned this pull request Jan 25, 2018

Make a "suggestions" comment type that can be "accepted" by the author #741

Open

2 tasks

		@@ -0,0 +1,6 @@
		<blockquote>
		<a href="/profile/<%= user %>"><%= user %></a> is suggesting an alternative title, "<%= title %>".

added "Suggest new title" feature #2000

added "Suggest new title" feature #2000

Conversation

ViditChitkara commented Jan 13, 2018 • edited Loading

ViditChitkara commented Jan 13, 2018

PublicLabBot commented Jan 13, 2018 • edited Loading

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViditChitkara commented Jan 16, 2018

jywarren commented Jan 16, 2018

ViditChitkara commented Jan 16, 2018

jywarren commented Jan 16, 2018

ViditChitkara commented Jan 17, 2018

jywarren commented Jan 17, 2018

ViditChitkara commented Jan 17, 2018 • edited Loading

ViditChitkara commented Jan 17, 2018

ViditChitkara commented Jan 17, 2018 • edited Loading

jywarren commented Jan 17, 2018

ViditChitkara commented Jan 18, 2018 via email

ViditChitkara commented Jan 18, 2018

jywarren commented Jan 19, 2018 via email

ViditChitkara commented Jan 20, 2018

ViditChitkara commented Jan 22, 2018

jywarren commented Jan 22, 2018

ViditChitkara commented Jan 22, 2018

ViditChitkara commented Jan 24, 2018

jywarren commented Jan 25, 2018

jywarren commented Jan 26, 2018

ViditChitkara commented Jan 26, 2018

namangupta01 commented Jan 26, 2018

ViditChitkara commented Jan 26, 2018 • edited Loading

namangupta01 commented Jan 26, 2018

jywarren commented Jan 26, 2018

ViditChitkara commented Jan 13, 2018 •

edited

Loading

PublicLabBot commented Jan 13, 2018 •

edited

Loading

ViditChitkara commented Jan 17, 2018 •

edited

Loading

ViditChitkara commented Jan 17, 2018 •

edited

Loading

ViditChitkara commented Jan 26, 2018 •

edited

Loading