Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 9 additions & 2 deletions app/controllers/term_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,15 @@
class TermController < ApplicationController
include Pagy::Backend

# This provides the list of terms that are awaiting confirmation.
# This provides the list of terms that are awaiting confirmation. By default this shows only terms which have been
# categorized automatically. Adding `type=all` to the querystring will show _all_ terms which the user has not yet
# confirmed.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

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.

def unconfirmed
@pagy, @records = pagy(Term.user_unconfirmed)
terms = if params[:show] == 'all'
Term.user_unconfirmed
else
Term.categorized.user_unconfirmed
end
@pagy, @records = pagy(terms)
end
end
1 change: 1 addition & 0 deletions app/models/term.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class Term < ApplicationRecord
before_save :register_fingerprint
after_destroy :check_fingerprint_count

scope :categorized, -> { where.associated(:categorizations).distinct }
scope :user_confirmed, -> { where.associated(:confirmations).distinct }
scope :user_unconfirmed, -> { where.missing(:confirmations).distinct }

Expand Down
5 changes: 5 additions & 0 deletions app/views/term/unconfirmed.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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">
Expand Down
17 changes: 17 additions & 0 deletions test/controllers/term_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,21 @@ class TermControllerTest < ActionDispatch::IntegrationTest

assert_response :success
end

test 'confirmation index can show two different sets of terms' do
sign_in users(:basic)
get terms_unconfirmed_path

# default_pagy will be something like "Displaying 10 items"
default_pagy = response.parsed_body.xpath('//main//span').first.text
default_pagy_count = default_pagy.split.second.to_i

get terms_unconfirmed_path(show: 'all')

# The '?type=all' route asks for more records, so the count should be higher
all_pagy = response.parsed_body.xpath('//main//span').first.text
all_pagy_count = all_pagy.split.second.to_i

assert_operator all_pagy_count, :>, default_pagy_count
end
end
26 changes: 26 additions & 0 deletions test/models/term_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No change necessary, but term_category_count as a variable name made me have to think more than felt necessary as to what is going on here. Maybe something like orig_category_count as it gets compared to the same count after a categorization is made might be better?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 orig_ prefix.

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
Expand Down