make keywords available for controlled vocabulary#1291
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds optional controlled-vocabulary support for keywords so events and materials can use the same dictionary-backed dropdown pattern already used for other vocabularies.
Changes:
- Add a new
KeywordsDictionaryclass and defaultconfig/dictionaries/keywords.yml. - Enable
keywordsas a supportedcontrolled_vocabulary_varsfeature in config comments. - Switch event/material keyword fields to use
f.dropdownwhen the controlled-vocabulary feature is enabled, and update the shared dropdown button label logic.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
config/tess.example.yml |
Documents keywords as an allowed controlled-vocabulary feature flag. |
config/dictionaries/keywords.yml |
Adds the default keyword dictionary file. |
app/views/materials/_form.html.erb |
Uses dictionary-backed dropdown keywords for materials when enabled. |
app/views/events/_form.html.erb |
Uses dictionary-backed dropdown keywords for events when enabled. |
app/views/common/_dropdown.html.erb |
Changes default dropdown button text generation to use the field label. |
app/dictionaries/keywords_dictionary.rb |
Adds the dictionary wrapper used to load keyword options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Dictionary of Keywords | ||
| class KeywordsDictionary < Dictionary | ||
|
|
||
| DEFAULT_FILE = 'keywords.yml' | ||
|
|
||
| private | ||
|
|
||
| def dictionary_filepath | ||
| get_file_path 'keywords', DEFAULT_FILE | ||
| end |
There was a problem hiding this comment.
should not be a concern as deployments with controlled vocabulary should not have the other values anyway
There was a problem hiding this comment.
I kinda agree with copilot. It is a bit weird to have this as a dictionary when really it just needs to be a list of values. If you wanted to have a controlled list of 50 keywords you would have to make up 50 random ids that are never actually used anywhere.
It could easily just be a txt file with each keyword separated by a linebreak, or a YAML file with just a list of strings. But maybe that would require too many changes?
fbacall
left a comment
fbacall
left a comment
There was a problem hiding this comment.
As mentioned on slack, maybe remove the keywords.yml dictionary since it just has placeholders
mikesndrs
force-pushed
the
feature/keywords_controlled_vocabulary
branch
from
632291a to
70d6272
Compare
fbacall
left a comment
fbacall
left a comment
There was a problem hiding this comment.
OK how about this:
-
We provide a default keywords dictionary called
keywords_example.ymlwith some dummy values that at least gives an indication of how keywords should be specified. -
If people want to implement their own keywords dictionary, they create e.g.
keywords.ymland change theirtess.ymlto point to this. Can add a comment tokeywords_example.ymlthat suggests this.
Also needs a test that attempts to load the form both with controlled_vocabulary_vars containing keywords and without.
| if File.exist?(dictionary_filepath) | ||
| YAML.safe_load(File.read(dictionary_filepath)).with_indifferent_access | ||
| else | ||
| {} | ||
| end |
There was a problem hiding this comment.
This would hide legit errors e.g. if you typo'd the dictionary name
| # Dictionary of Keywords | ||
| class KeywordsDictionary < Dictionary | ||
|
|
||
| DEFAULT_FILE = 'keywords.yml' | ||
|
|
||
| private | ||
|
|
||
| def dictionary_filepath | ||
| get_file_path 'keywords', DEFAULT_FILE | ||
| end |
There was a problem hiding this comment.
I kinda agree with copilot. It is a bit weird to have this as a dictionary when really it just needs to be a list of values. If you wanted to have a controlled list of 50 keywords you would have to make up 50 random ids that are never actually used anywhere.
It could easily just be a txt file with each keyword separated by a linebreak, or a YAML file with just a list of strings. But maybe that would require too many changes?
| private | ||
|
|
| <!-- Field: Keywords --> | ||
| <%= f.multi_input :keywords, errors: @event.errors[:keywords], | ||
| title: t('events.hints.keywords') %> | ||
| <% if TeSS::Config.feature['controlled_vocabulary_vars'].include? 'keywords' && File.exist?(KeywordsDictionary.instance.dictionary_filepath) %> |
|
|
||
| <!-- Field: Target Audience --> | ||
| <% if TeSS::Config.feature['controlled_vocabulary_vars'].include? 'target_audience' %> | ||
| <% if TeSS::Config.feature['controlled_vocabulary_vars'].include? 'target_audience' && File.exist?(TargetAudienceDictionary.instance.dictionary_filepath) %> |
Summary of changes
Enable controlled vocabulary dropdown for keywords
Checklist