Stop separatings tags by language #438

Oli0li · 2025-09-24T20:07:15Z

What Issue Does This PR Cover, If Any?

Resolves #400

What Changed? And Why Did It Change?

It changes the behaviour of Tags so they are no longer separated by language.

Because acts_as_taggable uses ActsAsTaggableOn::Tagging's "context" attribute to retrieve correct tags, it is not currently possible to use "topic.tags" to retrieve all the Tags associated to a Topic because our data has a mix of Taggings with "en" and "sp". For now, I've made changes so we can get tags to work even if the Tagging's context attribute is a language code. From now on, new tags will be created with "tags" as the Tagging's context, which will allow us to get the tags by using topic.tags. Once this is deployed and is confirmed to work as intended, we can fix the pre-existing records by changing all Taggings "context" to "tags". I will then open a follow-up PR to use "tags" instead of "base_tags".

If there are any pre-existing Tags that had cognates from both languages, we may need to fix the Topics tagged with them, as an English topic would only have been tagged with the English cognates, whereas from now on the topic would be expected to get the cognates from all languages

How Has This Been Tested?

With RSpec and by hand

Please Provide Screenshots

N/A

Additional Comments

N/A

Currently, Tags are associated to a Topic via a Tagging with a context set to the language code of the Topic. This commit makes changes so we ignore this context and get all associated tags regardless of context. Once this is merged and works with the current data, we'll fix the existing Taggings' "context" column to be "tags" so we can use acts_as_taggable methods such as "tags".

Since tagging is no longer divided by language, the #language_code method is no longer necessary.

We no longer want to only show tags matching the topic's language.

dmitrytrager · 2025-09-27T16:49:24Z

spec/services/csv_generator/tag_details_spec.rb


      before do
-        topic.tags_on(language.code.to_sym).each do |t|
+        topic.base_tags.each do |t|


Is this a method available directly from acts_as_taggable_on?

It is an association I found here:

https://stackoverflow.com/questions/52208139/how-to-get-all-tags-on-an-acts-as-taggable-object-regardless-of-its-contexts

And defined here:
https://github.com/mbleigh/acts-as-taggable-on/blob/8af75df3765001326443f36ed80dfde605b30edd/lib/acts-as-taggable-on/taggable.rb#L102

But I don't think it's meant to be used by users of the gem so this is just a temporary measure while we want to ignore the language contexts on the Taggings.

spec/services/text_generator/tags_spec.rb

db/seeds.rb

dmitrytrager · 2025-09-27T17:02:25Z

app/models/concerns/taggable.rb

-    return [] if language_tag_context.nil?

-    tags_on(language_tag_context)
+  # Issue 400: Can be changed to tags once we've changed all context attributes to "tags"


Since all tags now belongs to topic only, maybe change context to "topic"?

I was thinking of changing them to "tags" because that's the default value that the acts_as_taggable_on gem sets when using #tag_list= and that would allow us to use topic.tags:

https://github.com/mbleigh/acts-as-taggable-on/blob/8af75df3765001326443f36ed80dfde605b30edd/lib/acts-as-taggable-on/tagging.rb#L7

From what I understand reading the documentation and what I saw trying it in the console, if we set the context to a different value, we would have to use topic.tags_on(:topic) instead.

https://github.com/mbleigh/acts-as-taggable-on?tab=readme-ov-file#usage
https://github.com/mbleigh/acts-as-taggable-on?tab=readme-ov-file#dynamic-tag-contexts

Do we need to collect models (before using them in current_tag_list) because collection of tag models can give us tags from different contexts?

Sorry, I'm not sure I understand the question. Are you asking if we are defining current_tag_list instead of just using tag_list because of the fact that we currently have different context values? If so, yes, and we should be able to use tag_list as soon as we have updated all current context values to "tags".

Here is what I saw testing it in the console:

With a Tagging with a "tags" context:

With a Tagging with an "en" context:

Dmitry, let me know if this resolves the conversation.

@dcollie2 my latest suggestion is described here

dmitrytrager · 2025-09-29T22:22:53Z

app/models/concerns/taggable.rb

-    return [] if language_tag_context.nil?

-    tags_on(language_tag_context)
+  # Issue 400: Can be changed to tags once we've changed all context attributes to "tags"


Do we need to collect models (before using them in current_tag_list) because collection of tag models can give us tags from different contexts?

app/javascript/controllers/select_tags_controller.js

dmitrytrager · 2025-09-29T22:37:00Z

app/models/concerns/taggable.rb

-
-  def language_code
-    language.code.to_sym
+  # Issue 400: Can be changed to all_tags_list once we've changed all context attributes to "tags"


How are we going to change context attributes to "tags" for existing records?

I was thinking of doing ActsAsTaggableOn::Tagging.update_all(context: "tags") in the console. Once this PR is merged, any new tagging will be created with a "tags" context. Using #base_tags until the context has been updated on all preexisting Taggings will allow us to display/manage old and new Taggings regardless of the value of their context.

What if we add rake task to this PR that will change context for existing record.
We will run it as a post-deploy step.
In this case data will be ready to work with one single context.

Assuming that we won't need to work with models and can use methods like tag_list, is it correct?

I'm fine with either approach: do the rake task & fix on post-deploy or merge this as an incremental step.

Just don't see any reason to postpone this change. Additionally it should simplify our approach for working with tags

I just tried replacing all #current_tags with #tags and #current_tags_list with #tag_list, but these 3 tests error:

For now I only looked into the ones in the topics/update_spec file but it is not clear to me why they error. #tag_list is correct after saving but goes back to being incorrect when I do reload.tag_list, and I can see the Tagging is not destroyed even though it should. But when I test it by hand, I have no problem removing tags so I don't know if this is just an issue with the test.

Unfortunately, I won't be able to look more into this before I go abroad tomorrow for a week. So feel free to look into this if you have time, if not I'll look into it when I am back at the end of next week. In the meantime, I didn't see any problem when testing the behaviour for what I've changed in this PR, so I think it should be safe to merge it, but it would be great if you could take a look as well.

No worries. I will look into

Thank you, @dmitrytrager!

dmitrytrager · 2025-10-06T22:29:52Z

I've added migration, so all contexts are merged into "tags".
And then removed methods where we work with context, now using tag lists directly

Oli0li · 2025-10-09T17:28:03Z

I've added migration, so all contexts are merged into "tags". And then removed methods where we work with context, now using tag lists directly

Thank you so much @dmitrytrager! I think we could directly call the methods provided by the acts-as-taggable-on gem now so I've added a commit to do that, but can remove if you think wrapping these methods with our own names is better.

I tested it by hand and didn't see any issue.

dmitrytrager · 2025-10-10T20:43:53Z

Perfect, thanks

dcollie2

I want to coordinate with you before merging.

Oli0li requested review from dcollie2, dmitrytrager and hernanvicente September 24, 2025 20:18

Oli0li added 8 commits September 24, 2025 21:19

Remove #language_code

3be7628

Since tagging is no longer divided by language, the #language_code method is no longer necessary.

Remove unused #current_tags_for_language method

9daf0c7

Remove language context from SynchronizeCognatesOnTopicsJob

e4717e6

Remove language context from DataImport

2f8c62e

Remove language context from seeds and tests

f374987

Amend Topic form to show all tags

784a73d

We no longer want to only show tags matching the topic's language.

Fix lint issues

f598cb2

Oli0li force-pushed the 400-no-longer-divide-tags-by-language branch from 7d6f3ea to f598cb2 Compare September 24, 2025 20:19

dmitrytrager reviewed Sep 27, 2025

View reviewed changes

Oli0li added 2 commits September 29, 2025 12:51

Move tag assignment outside of loop

606d7a8

Use #map to avoid repetition in seeds

58e0b01

Oli0li requested a review from dmitrytrager September 29, 2025 14:44

dmitrytrager reviewed Sep 29, 2025

View reviewed changes

Oli0li requested a review from dmitrytrager September 30, 2025 14:41

dmitrytrager added 5 commits October 4, 2025 22:47

Add migration to change taggings context

67b9f16

Update methods from taggable

b2c5596

Commit schema change

5a12dee

Update #process_tags logic and fix specs

438b372

Do not use #set_tag_list_on with language context

7d32827

Use the tag methods provided by acts-as-taggable-on

6ff2d71

dcollie2 approved these changes Oct 25, 2025

View reviewed changes

Uh oh!

Stop separatings tags by language #438

Are you sure you want to change the base?

Stop separatings tags by language #438

Conversation

Oli0li commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Issue Does This PR Cover, If Any?

What Changed? And Why Did It Change?

How Has This Been Tested?

Please Provide Screenshots

Additional Comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oli0li Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oli0li Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oli0li Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrytrager commented Oct 6, 2025

Uh oh!

Oli0li commented Oct 9, 2025

Uh oh!

dmitrytrager commented Oct 10, 2025

Uh oh!

dcollie2 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Oli0li commented Sep 24, 2025 •

edited

Loading

Oli0li Sep 29, 2025 •

edited

Loading

Oli0li Sep 29, 2025 •

edited

Loading

Oli0li Sep 30, 2025 •

edited

Loading