Created a locked tag which can be added to notes by mods / admin #9709

Manasa2850 · 2021-05-31T10:43:54Z

Part of #9694

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

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

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

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

Thanks!

gitpod-io · 2021-05-31T10:43:56Z

codecov · 2021-05-31T10:49:18Z

Codecov Report

❗ No coverage uploaded for pull request base (main@ece2611). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #9709   +/-   ##
=======================================
  Coverage        ?   82.06%           
=======================================
  Files           ?       98           
  Lines           ?     5931           
  Branches        ?        0           
=======================================
  Hits            ?     4867           
  Misses          ?     1064           
  Partials        ?        0

Manasa2850 · 2021-05-31T10:50:28Z

I would like some suggestions regarding how the 'Lock Note' button should look. Thanks!

Manasa2850 · 2021-05-31T16:15:02Z

@jywarren @RuthNjeri this is how it looks when a normal user tries to delete the 'locked' tag on their content. I've changed the message to Only admins can delete the locked tag.

Another way of implementing this, is by not giving the delete tag option to normal users for the 'locked' tag. But this might cause some confusion to users as to why they can't delete tags on their own notes. Please let me know your thoughts regarding this.

app/controllers/notes_controller.rb

config/routes.rb

app/views/notes/show.html.erb

jywarren · 2021-06-01T16:19:57Z

I love the "can't delete" message you've added. Great work!

Manasa2850 · 2021-06-01T17:32:01Z

Thanks @jywarren for reviewing! I'll take a look at these suggestions and try implementing them.

codeclimate · 2021-06-02T10:08:21Z

Code Climate has analyzed commit 58e59b8 and detected 0 issues on this pull request.

View more on Code Climate.

Manasa2850 · 2021-06-02T10:22:00Z

app/assets/javascripts/tagging.js

@@ -140,7 +140,7 @@ function promptTag(val) {
      break;

    default:
-      addTag(expr);
+      addTag(val);


This change is not directly related to the 'locked' tag but is required in the promptTag function.

Ah, was it a bug from before? Cool. Thanks!

jywarren · 2021-06-02T21:27:42Z

app/views/notes/show.html.erb

@@ -86,6 +86,10 @@
    </table>
  <% end %>

+  <% if current_user && ( current_user.role == "admin" || current_user.role == "moderator") && !(@node.has_tag('locked')) %>
+    <a href="javascript:window.location.reload(true)" onClick="addTag('locked');">Lock Note</a>


I think this may be an issue in Firefox, but I could be wrong. I noted this in #9726 too!

@jywarren I use Firefox and it works fine!

@Manasa2850 I just tested the Lock Note option on the stable site, and on the first click, the reload does not add the tag, I have to click it again or manually reload the page. I am using a chrome browser.

Screen.Recording.2021-06-10.at.17.35.10.mov

@RuthNjeri I'm not really sure why that's happening on the stable site. I tried it out on localhost both on Chrome as well as Firefox and the tag gets added on reload. There's no need to refresh the page.

I'll take a deeper look at this to find out why that's happening on the stable site.

I also think you need to be an admin to test this out on the stable site, I don't know how to make someone an admin on the stable site, but if you share your username with Jeff, he will be able to do that.

My username on the stable site is Manasa2850.

I figured out how to make you an admin, you are now one.

@RuthNjeri I tried it out on stable and this is what I observer

On Firefox - it works perfectly fine. We don't need to refresh the page for the locked tag to get added.

On Chrome - sometimes it works fine, without requiring a refresh. But few other times, on clicking the Lock Note button, the note gets locked (i.e the lock favicon appears and the Lock Note button goes away) but the 'locked' tag card doesn't appear in the sidebar. We need to refresh the page in order to see it on the sidebar.

I will try to find out why this is happening just on Chrome. Thanks!

Thanks for the update @Manasa2850

jywarren

Just a note on Firefox usage. This looks really good and a bit simpler too! Love it!!!

Manasa2850 · 2021-06-03T07:47:31Z

@jywarren I use Firefox and it works fine!

jywarren · 2021-06-03T11:38:37Z

Sounds great. Merging this! Excellent work!

jywarren · 2021-06-03T11:41:30Z

app/views/notes/show.html.erb

@@ -86,6 +86,10 @@
    </table>
  <% end %>

+  <% if current_user && ( current_user.role == "admin" || current_user.role == "moderator") && !(@node.has_tag('locked')) %>


What if, in follow up, we moved this button into the advanced tagging menu? This would then appear on wikis too!

RuthNjeri · 2021-06-03T11:43:12Z

app/controllers/tag_controller.rb

@@ -359,7 +359,7 @@ def delete
    node_tag = NodeTag.where(nid: params[:nid], tid: params[:tid]).first
    node = Node.where(nid: params[:nid]).first
    # only admins, mods, and tag authors can delete other peoples' tags
-    if node_tag.uid == current_user.uid || logged_in_as(['admin', 'moderator']) || node.uid == current_user.uid
+    if node_tag.uid == current_user.uid || logged_in_as(['admin', 'moderator']) || (node.uid == current_user.uid && node_tag.name != "locked")


From line 359 above, we have the node_tag variable derived from this query node_tag = NodeTag.where(nid: params[:nid], tid: params[:tid]).first ,

I am curious as to whether checking to see if node_tag.name != "locked" will work if the first tag derived is not "locked" but contains a list of tags like [air-quality, locked]....the node itself does contain a locked tag but it may not be the first to be picked hence someone else might delete a tag.

Should checking if the tags returned contain "locked", before picking the first one in line 359 help?

lol I was just reviewing this and panicked that I had merged it @jywarren 😅

Are the concerns I have above okay? Or will the tagging work as expected... @jywarren?

Oh you're totally right, my bad! Yes, let's change this, we can use @node.has_tag('locked') instead!

@Manasa2850 what do you say? Thanks all!

Manasa2850 · 2021-06-03T17:02:30Z

@jywarren I like your idea of moving the lock button to the advanced menu, but I feel it should be there on the main page too since that's a more obvious place for admins / mods to see. Let's have the button in both places. What do you think?

@RuthNjeri @jywarren sure I'll put a condition for the 'locked' tag so that it works even when it's not the first one.
Thanks!!

Manasa2850 · 2021-06-03T17:37:14Z

@RuthNjeri @jywarren sorry I might be wrong but line 359 node_tag = NodeTag.where(nid: params[:nid], tid: params[:tid]).first refers to the 'locked' tag when we click the delete tag option on the 'locked' tag right? Because tid is unique for every tag we create. So when we click on the delete tag option on the 'locked' tag, node_tag will correspond to the 'locked' tag itself. I don't think it will make a difference whether 'locked' is the first tag in the list of tags or not.

I tried it out when 'locked' is not the first tag and it seems to be working fine.
Please do let me know if I'm missing out on something here. Thanks!

RuthNjeri · 2021-06-10T14:08:15Z

@RuthNjeri @jywarren sorry I might be wrong but line 359 node_tag = NodeTag.where(nid: params[:nid], tid: params[:tid]).first refers to the 'locked' tag when we click the delete tag option on the 'locked' tag right? Because tid is unique for every tag we create. So when we click on the delete tag option on the 'locked' tag, node_tag will correspond to the 'locked' tag itself. I don't think it will make a difference whether 'locked' is the first tag in the list of tags or not.

I tried it out when 'locked' is not the first tag and it seems to be working fine.
Please do let me know if I'm missing out on something here. Thanks!

Hi Manasa, yes you are right, if someone clicks on the locked tag to try and delete it, the tid in the query will be that of the locked tag. However, let us say a user is trying to delete another tag, lets say air quality and that node, for instance a research note, has the locked tag present, the tag retrieved from the delete is the air quality and it might get deleted because we have not checked if the current node has an additional locked tag...

From my understanding, a research note can have many tags and in order to freely delete any of the tags, it must not contain the locked tag...

* added test for adding locked tag * admins can add locked tag test * test for admin can add locked tag * added test for moderator * made changes suggested in PR #9709

…liclab#9709) * adding locked tag * normal users can't delete locked tag * changed error message * added javascript function for locking

* added test for adding locked tag * admins can add locked tag test * test for admin can add locked tag * added test for moderator * made changes suggested in PR publiclab#9709

…liclab#9709) * adding locked tag * normal users can't delete locked tag * changed error message * added javascript function for locking

* added test for adding locked tag * admins can add locked tag test * test for admin can add locked tag * added test for moderator * made changes suggested in PR publiclab#9709

adding locked tag

bdabbac

Manasa2850 added the outreachy label May 31, 2021

normal users can't delete locked tag

0eab1d1

Manasa2850 requested review from RuthNjeri and jywarren May 31, 2021 16:15

changed error message

52a981b

jywarren mentioned this pull request Jun 1, 2021

[Planning] Tag / Topic System Enhancement Project #9668

Open

15 tasks

jywarren reviewed Jun 1, 2021

View reviewed changes

app/controllers/notes_controller.rb Outdated Show resolved Hide resolved

jywarren reviewed Jun 1, 2021

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

jywarren reviewed Jun 1, 2021

View reviewed changes

app/views/notes/show.html.erb Outdated Show resolved Hide resolved

added javascript function for locking

58e59b8

Manasa2850 commented Jun 2, 2021

View reviewed changes

jywarren reviewed Jun 2, 2021

View reviewed changes

jywarren requested changes Jun 2, 2021

View reviewed changes

Manasa2850 requested a review from jywarren June 3, 2021 07:46

jywarren merged commit 6ad3102 into publiclab:main Jun 3, 2021

jywarren reviewed Jun 3, 2021

View reviewed changes

RuthNjeri reviewed Jun 3, 2021

View reviewed changes

Manasa2850 mentioned this pull request Jun 3, 2021

[Outreachy Planning] Tag / Topic System Enhancement Project - Manasa #9694

Open

11 tasks

RuthNjeri mentioned this pull request Jun 9, 2021

Tags added by banned users are not displayed #9757

Merged

5 tasks

Manasa2850 added a commit to Manasa2850/plots2 that referenced this pull request Jun 13, 2021

made changes suggested in PR publiclab#9709

5d74ede

Manasa2850 mentioned this pull request Jun 13, 2021

Tests for tag locking #9772

Merged

5 tasks

RuthNjeri pushed a commit that referenced this pull request Jun 20, 2021

Tests for tag locking (#9772)

bd930ec

* added test for adding locked tag * admins can add locked tag test * test for admin can add locked tag * added test for moderator * made changes suggested in PR #9709

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created a locked tag which can be added to notes by mods / admin #9709

Created a locked tag which can be added to notes by mods / admin #9709

Manasa2850 commented May 31, 2021 •

edited

Loading

gitpod-io bot commented May 31, 2021

codecov bot commented May 31, 2021 •

edited

Loading

Manasa2850 commented May 31, 2021

Manasa2850 commented May 31, 2021 •

edited

Loading

jywarren commented Jun 1, 2021

Manasa2850 commented Jun 1, 2021

codeclimate bot commented Jun 2, 2021

Manasa2850 Jun 2, 2021

jywarren Jun 2, 2021

jywarren Jun 2, 2021

Manasa2850 Jun 3, 2021

RuthNjeri Jun 10, 2021

Manasa2850 Jun 10, 2021

RuthNjeri Jun 11, 2021

Manasa2850 Jun 11, 2021

RuthNjeri Jun 11, 2021

Manasa2850 Jun 11, 2021 •

edited

Loading

RuthNjeri Jun 20, 2021

jywarren left a comment

Manasa2850 commented Jun 3, 2021

jywarren commented Jun 3, 2021

jywarren Jun 3, 2021

RuthNjeri Jun 3, 2021

RuthNjeri Jun 3, 2021

jywarren Jun 3, 2021

jywarren Jun 3, 2021

Manasa2850 commented Jun 3, 2021

Manasa2850 commented Jun 3, 2021

RuthNjeri commented Jun 10, 2021

Created a locked tag which can be added to notes by mods / admin #9709

Created a locked tag which can be added to notes by mods / admin #9709

Conversation

Manasa2850 commented May 31, 2021 • edited Loading

gitpod-io bot commented May 31, 2021

codecov bot commented May 31, 2021 • edited Loading

Codecov Report

Manasa2850 commented May 31, 2021

Manasa2850 commented May 31, 2021 • edited Loading

jywarren commented Jun 1, 2021

Manasa2850 commented Jun 1, 2021

codeclimate bot commented Jun 2, 2021

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

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

Manasa2850 Jun 11, 2021 • edited Loading

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

Manasa2850 commented Jun 3, 2021

jywarren commented Jun 3, 2021

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

Manasa2850 commented Jun 3, 2021

Manasa2850 commented Jun 3, 2021

RuthNjeri commented Jun 10, 2021

Manasa2850 commented May 31, 2021 •

edited

Loading

codecov bot commented May 31, 2021 •

edited

Loading

Manasa2850 commented May 31, 2021 •

edited

Loading

Manasa2850 Jun 11, 2021 •

edited

Loading