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.

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

Revamp/deprecate tag aliasing, migrate aliases to new tags #6367

Open
1 of 6 tasks
jywarren opened this issue Oct 1, 2019 · 12 comments
Open
1 of 6 tasks

Revamp/deprecate tag aliasing, migrate aliases to new tags #6367

jywarren opened this issue Oct 1, 2019 · 12 comments
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration deprecation feature explains that the issue is to add a new feature hacktoberfest help wanted requires help by anyone willing to contribute planning Planning issues!

Comments

@jywarren
Copy link
Member

jywarren commented Oct 1, 2019

We have 3 scenarios which relate tags to one another:

  1. "incidental need to show 2 topics"
    - all good: https://publiclab.org/search?q=water+education, https://publiclab.org/search?q=water-education
    - https://booleanstrings.com/2018/03/08/the-full-list-of-google-advanced-search-operators/
  2. "these are the same"
  3. "this belongs under that"

Our current "tag aliasing" system is very fragile and has lots of cumbersome code, and is hard to maintain.

Proposal: migrate tag alias system to automated re-tagging system

  • add preferred tag to all sub-tags (using a script)
  • resubscribe people to the larger tag
  • shut down aliasing, remove code
  • real-time auto-tag additions (from a list) - break this out, as it's complex
    • source from main topics list (subtopics from footer) and related tags list (for search - Call for related search terms #4928 ) (synonyms) and auto-add these whenever a subtag is added by anyone

Notes on different solutions to this issue:

1: https://publiclab.org/tag/legal+evidence+permits+regulations
- (ALL, or any content that has ALL those tags) - logical operators?
does this mean restricting results in this URL to content that only has all four of these tags?
- what's useful?
2: https://publiclab.org/wiki/combined-legal grid:
- [notes:legal+evidence+permits+regulations] (manually doing it on a wiki page)
useful?
3: tag aliasing: https://publiclab.org/tag/legal (where evidence+permits+regulations are aliased from it...)
- content tagged 'oil' already appears to be showing up on /tag/oil-and-gas, so i suppose this is aliased already?
- This is an old feature that doubles the work that the server has to do. Currently doubling the amount of work that goes into any feature.
4: manual re-tagging
- bad --> i would like to revisit this adjective.
5: Automated Re-tagging
- write a script that adds "oil-and-gas" to any "oil" or any "gas" post
6: real-time auto-tagging - at publish time

for 6, we can expand this:

@jywarren jywarren added help wanted requires help by anyone willing to contribute break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration feature explains that the issue is to add a new feature planning Planning issues! labels Oct 1, 2019
@jywarren jywarren added this to the Tagging and topics milestone Oct 1, 2019
@ebarry
Copy link
Member

ebarry commented Oct 1, 2019

This is great!

@manishaag7
Copy link
Contributor

Can I work on this @jywarren ?

@jywarren
Copy link
Member Author

You're welcome to begin on part of it! It's a big project! For example the following would be done through a migration script in /db/migrate/..... -- we'd have to find any instances where a tag record has a parent field that's not empty, then:

  • add the named parent tag to all content bearing the original tags
  • remove the parent field from the original tag

Code currently driving this system is mostly in the tag.rb file (which we'd then want to remove):

plots2/app/models/tag.rb

Lines 105 to 111 in d7a021d

parents = Node.where(status: 1, type: type)
.includes(:tag)
.references(:term_data)
.where('term_data.name IN (?)', tags.collect(&:parent))
order = 'node_revisions.timestamp DESC'
order = 'created DESC' if type == 'note'
Node.where('node.nid IN (?)', (nodes + parents).collect(&:nid))

However, first would we want to re-subscribe people? I guess I think not. The tags will both still exist. If we want to do something like this in follow-up, we can.

Critically here, let's /keep/ a list of all tags that have parents so we can track it. Here, i ran this in the console on the production server:

irb(main):002:0> t = Tag.where.not(parent: nil).count
=> 10

Here they are as tag parent pairs:

pm air-quality
h2s hydrogen-sulfide
near-infrared-camera multispectral-imaging
infragram multispectral-imaging
odors odor
oil oil-and-gas
purple-air purpleair
reagent reagents
spectrometry spectrometer
spectrometer spectrometry

@jywarren
Copy link
Member Author

Then, we'd want to add these all to this list of subtags:

plots2/app/models/node.rb

Lines 850 to 855 in d7a021d

# add sub-tags:
subtags = {}
subtags['pm'] = 'particulate-matter'
if subtags.include?(key)
add_tag(subtags[key], user)
end

Actually that makes a nice first-timers-only issue!

@jywarren
Copy link
Member Author

Made it here: #9398

@jywarren
Copy link
Member Author

For removing the code, we can do that now, and it's the following locations:

  • any instance of the word "parent" in tag.rb!
  • def add_parent
    if logged_in_as(['admin'])
    @tag = Tag.find_by(name: params[:name])
    @tag.update_attribute('parent', params[:parent])
    if @tag.save
    flash[:notice] = "Tag parent added."
    else
    flash[:error] = "There was an error adding a tag parent."
    end
    redirect_to '/tag/' + @tag.name + '?_=' + Time.now.to_i.to_s
    else
    flash[:error] = "Only admins may add tag parents."
    end
    end
  • get 'tag/parent' => 'tag#add_parent'
  • before_action :require_user, only: %i(create delete add_parent)

Removing the above should cause the following tests to fail, but we should just then remove these tests:

  • test 'aliasing of tag specified in tag.parent' do
    node = nodes(:one)
    assert_equal tags(:spectrometer).parent, 'spectrometry'
    assert_nil tags(:spectrometry).parent
    assert node.has_tag('spectrometer') # this is directly true
    assert node.has_tag('spectrometry') # this true via aliasing
    assert node.has_tag_without_aliasing('spectrometer')
    assert_not node.has_tag_without_aliasing('spectrometry')
    assert_equal node.get_matching_tags_without_aliasing('spectrometer').length, 1
    assert_equal node.get_matching_tags_without_aliasing('spectrometry').length, 0
    assert_not Tag.find_nodes_by_type('spectrometry').to_a.include?(node)
    assert_not Tag.find_nodes_by_type_with_all_tags(['spectrometry']).to_a.include?(node)
    assert Tag.find_nodes_by_type('spectrometer').to_a.include?(node)
    assert Tag.find_nodes_by_type_with_all_tags(['spectrometer']).to_a.include?(node)
    # test node.add_tag, which uses has_tag
    saved, tag = node.add_tag('spectrometry', users(:bob))
    assert saved
    assert_not_nil tag
    end
    test 'aliasing of tags which have parent matching initial tag' do
    node = nodes(:one)
    tag = tags(:spectrometry)
    tag.parent = 'spectrometer'
    tag.save
    tag2 = tags(:spectrometer)
    tag2.parent = ''
    tag2.save
    assert node.has_tag('spectrometer') # this is directly true
    assert_not node.has_tag('spectrometry') # should return false; <spectrometer>.parent == ""
    assert Tag.find_nodes_by_type('spectrometer').to_a.include?(node)
    assert Tag.find_nodes_by_type_with_all_tags(['spectrometer']).to_a.include?(node)
    assert Tag.find_nodes_by_type('spectrometry').to_a.include?(node)
    assert Tag.find_nodes_by_type_with_all_tags(['spectrometry']).to_a.include?(node)
    end
    test 'aliasing of cross-parented tags' do
    node = nodes(:one)
    tag = tags(:spectrometry)
    tag.parent = 'spectrometer'
    tag.save
    tag2 = tags(:spectrometer)
    tag2.parent = 'spectrometry'
    tag2.save
    assert node.has_tag('spectrometer') # this is directly true
    assert node.has_tag('spectrometry') # should return true even if the node only has tag 'spectrometer'
    assert Tag.find_nodes_by_type('spectrometry').to_a.include?(node)
    assert Tag.find_nodes_by_type_with_all_tags(['spectrometry']).to_a.include?(node)
    end
  • assert_equal tags(:spectrometer).parent, 'spectrometry'
    # iterate through results
    assert !assigns['notes'].empty?
    assigns['notes'].each do |node|
    assert node.has_tag('spectrometry') # should return false
    assert_not node.has_tag_without_aliasing('spectrometry') # should return false
    end
  • test 'can create tag instance (community_tag) using a parent tag' do
    UserSession.create(users(:bob))
    post :create, params: { name: 'spectrometry', nid: nodes(:one).nid, uid: users(:bob).id }
    assert_equal 'spectrometry', assigns[:tags].last.name
    assert_redirected_to(nodes(:one).path)
    end
    test 'shows things tagged with child tag' do
    tag = tags(:spectrometer)
    tag.parent = 'spectrometry'
    tag.save
    tag2 = tags(:spectrometry)
    tag2.parent = ''
    tag2.save
    assert_equal 'spectrometry', tag.parent
    assert_equal '', tag2.parent
    nodes(:blog).add_tag('spectrometry', users(:bob))
    assert nodes(:blog).has_tag_without_aliasing('spectrometry')
    get :show, params: { id: 'spectrometry' }
    # order of timestamps during testing (almost same timestamps) was causing testing irregularities
    notes = assigns(:notes).sort_by(&:title).reverse
    assert_equal 2, notes.length
    assert_equal [1, 13], notes.collect(&:nid)
    assert_equal [nodes(:one).title, 'Blog post'], notes.collect(&:title)
    # should be the first node, nid=1
    assert_equal nodes(:one).title, notes.first.title
    assert_equal ['spectrometer'], notes.first.tags.collect(&:name)
    assert notes.first.has_tag_without_aliasing('spectrometer')
    assert_not notes.first.has_tag_without_aliasing('spectrometry')
    # should be the blog node, nid=13
    assert_equal 'Blog post', notes.last.title
    assert_equal ['spectrometry'], notes.last.tags.collect(&:name)
    assert_not notes.last.has_tag_without_aliasing('spectrometer')
    assert notes.last.has_tag_without_aliasing('spectrometry')
    end
    test 'does not show things tagged with parent tag' do
    tag = tags(:spectrometer)
    tag.parent = 'spectrometry'
    tag.save
    tag2 = tags(:spectrometry)
    tag2.parent = ''
    tag2.save
    assert_equal 'spectrometry', tags(:spectrometer).parent
    assert_equal '', tags(:spectrometry).parent
    nodes(:blog).add_tag('spectrometry', users(:bob))
    get :show, params: { id: 'spectrometer' }
    assert_equal 1, assigns(:notes).length
    assert_not assigns(:notes).first.has_tag_without_aliasing('spectrometry')
    assert assigns(:notes).first.has_tag_without_aliasing('spectrometer')
    end
  • test 'add_parent method adds a tag parent' do
    user = UserSession.create(users(:admin))
    get :add_parent, params: { name: Tag.last.name, parent: Tag.first.name }
    assert_response :redirect
    assert_equal Tag.first.name, Tag.last.parent
    # flash[:notice] = "Tag parent added."
    # flash[:error] = "There was an error adding a tag parent."
    # redirect_to '/tag/' + @tag.name + '?_=' + Time.now.to_i.to_s
    end
    test 'add_parent method works with non-existent parent' do
    user = UserSession.create(users(:admin))
    get :add_parent, params: { name: Tag.last.name, parent: Tag.first.name }
    assert_response :redirect
    assert_equal Tag.first.name, Tag.last.parent
    get :index
    assert_response :success
    end
  • # Bug 6855
    test 'counts match the nodes for a parent tag' do
    tag = tags(:sun)
    get :show, params: { id: tag.name }
    assert_response :success
    counts = assigns(:counts)
    assert_equal 2, counts[:posts], "Note count should match"
    assert_equal 1, counts[:questions], "Question count should match"
    assert_equal 2, counts[:wiki], "Wiki count should match"
    assert_equal 2, assigns(:total_posts), "Total posts should match"
    end

@jcads
Copy link
Contributor

jcads commented Mar 30, 2021

any instance of the word "parent" in tag.rb!

Few questions,

  • Is "parent" the same as "parents"?
  • Should this be removed?

    plots2/app/models/tag.rb

    Lines 105 to 108 in d7a021d

    parents = Node.where(status: 1, type: type)
    .includes(:tag)
    .references(:term_data)
    .where('term_data.name IN (?)', tags.collect(&:parent))

pinging @jywarren

@manishaag7
Copy link
Contributor

manishaag7 commented Mar 31, 2021

@jywarren is @jcads will be working on this or I can work on it?
If not , I would like to work on this.

@jcads
Copy link
Contributor

jcads commented Apr 1, 2021

Hello @manishaag7, I have already opened a PR (#9403) for this particular issue. But as @jywarren said, you can work on this as this is a big project.

@manishaag7
Copy link
Contributor

ok thanks @jcads

@jywarren
Copy link
Member Author

jywarren commented May 4, 2021

  • Is "parent" the same as "parents"?
  • Should this be removed?

Yes, that's correct! Sorry for the slow reply!!! Thank you! And thanks @manishaag7 we appreciate all the help we can get. 🎉

@jywarren
Copy link
Member Author

jywarren commented May 4, 2021

@jcads yours got merged in #9404 instead, thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration deprecation feature explains that the issue is to add a new feature hacktoberfest help wanted requires help by anyone willing to contribute planning Planning issues!
Projects
None yet
Development

No branches or pull requests

6 participants