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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Jun 3, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bdabbac
adding locked tag
Manasa2850 May 31, 2021
0eab1d1
normal users can't delete locked tag
Manasa2850 May 31, 2021
52a981b
changed error message
Manasa2850 Jun 1, 2021
58e59b8
added javascript function for locking
Manasa2850 Jun 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/assets/javascripts/tagging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ function promptTag(val) {
break;

default:
addTag(expr);
addTag(val);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

break;

}
Expand Down
5 changes: 4 additions & 1 deletion app/controllers/tag_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@Manasa2850 what do you say? Thanks all!


tag = Tag.joins(:node_tag)
.select('term_data.name')
Expand All @@ -385,6 +385,9 @@ def delete
end
end
end
elsif node_tag.name == "locked"
flash[:error] = "Only admins can delete the locked tag."
redirect_to Node.find_by(nid: params[:nid]).path
else
flash[:error] = I18n.t('tag_controller.must_own_tag_to_delete')
redirect_to Node.find_by(nid: params[:nid]).path
Expand Down
4 changes: 4 additions & 0 deletions app/views/notes/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@
</table>
<% end %>

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

Choose a reason for hiding this comment

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

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

<a href="javascript:window.location.reload(true)" onClick="addTag('locked');">Lock Note</a>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@jywarren I use Firefox and it works fine!

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.
ezgif com-gif-maker

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

My username on the stable site is Manasa2850.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Manasa2850 Manasa2850 Jun 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

stable-firefox

  1. 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.

stable-chrome

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the update @Manasa2850

<% end %>

<% if current_user &&
( current_user.role == "admin" || current_user.role == "moderator" ||
current_user.id == @node.id || current_user.is_coauthor?(@node) ) &&
Expand Down