-
Notifications
You must be signed in to change notification settings - Fork 0
Add option to confirm only categorized terms #174
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,11 @@ | |
| <p>The terms listed here have not yet been reviewed by a person and placed into a category. Clicking on any term will | ||
| take you to the form for submitting this information.</p> | ||
|
|
||
| <p class="wrap-filters">View: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this UI option should be displayed to all users. I believe our goal is to just have people validate for the foreseeable future and this may make that slightly more confusing. I'd recommend either hiding this behind admin capabilities or removing it entirely. Having the feature available I like...presenting the option to all logged in users feels like we are possibly inviting trouble.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated the implementation to enact a restriction like this, so basic users don't see (and can't use) the broader scope. I've also written a number of new controller tests to confirm that things behave the way I expect. I'm not particularly happy with having a test that's this tightly coupled to UI text, but I'd rather this than not have a test for it at all. |
||
| <a class="btn button-small button-secondary" href="<%= terms_unconfirmed_path %>">Categorized terms</a> | ||
| <a class="btn button-small button-secondary" href="<%= terms_unconfirmed_path(show: 'all') %>">All terms</a> | ||
| </p> | ||
|
|
||
| <p><%== pagy_info(@pagy) %></p> | ||
|
|
||
| <ul class="list-unbulleted"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -299,6 +299,32 @@ class TermTest < ActiveSupport::TestCase | |
| end | ||
| end | ||
|
|
||
| test 'categorized scope returns an active record relation' do | ||
| assert_kind_of ActiveRecord::Relation, Term.categorized | ||
| end | ||
|
|
||
| test 'categorized scope accounts for terms with multiple categorizations' do | ||
| categorized_count = Term.categorized.count | ||
| t = terms('doi') | ||
|
|
||
| term_category_count = t.categorizations.count | ||
|
|
||
| # term has been categorized already | ||
| assert_operator 1, :<=, term_category_count | ||
|
|
||
| new_record = { | ||
| term: t, | ||
| category: categories('navigational'), | ||
| confidence: 0.5, | ||
| detector_version: '1' | ||
| } | ||
| Categorization.create!(new_record) | ||
|
|
||
| # The term has gained a category, but the categorized scope has not changed size. | ||
| assert_operator term_category_count, :<, t.categorizations.count | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No change necessary, but
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I can see how a different name might be better. I've tried to make it something clearer, and I particularly like the |
||
| assert_equal categorized_count, Term.categorized.count | ||
| end | ||
|
|
||
| test 'user_confirmed scope returns an active record relation' do | ||
| assert_kind_of ActiveRecord::Relation, Term.user_confirmed | ||
| end | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to change for this PR as the default is what we expect people to use for the foreseeable future, but it might be better longterm to make this a sticky option by setting it as a session variable or cookie. I don't even think we need a ticket for that as we may never come back to it, but just wanted to share that the ergonomics of this is a bit awkward if someone is trying to "categorize" terms rather than validate categorizations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, having the choice be sticky seems like something that could be useful in the future, or even a workflow that just picks terms according to some logic, so that the user never goes back to a list-of-terms page.
Regardless, I agree that this is a concern for the future should we ever really start using this workflow.