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

AO3-3471 Filter works per language on language_works_path #4659

Merged
merged 9 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
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
7 changes: 1 addition & 6 deletions app/controllers/languages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@ def index
@languages = Language.default_order
end

def show
@language = Language.find_by(short: params[:id])
@works = @language.works.recent.visible.limit(ArchiveConfig.NUMBER_OF_ITEMS_VISIBLE_IN_DASHBOARD)
end

def new
@language = Language.new
authorize @language
Expand All @@ -35,7 +30,7 @@ def update
authorize @language
if @language.update(language_params)
flash[:notice] = t('successfully_updated', default: 'Language was successfully updated.')
redirect_to @language
redirect_to languages_path
else
render action: "new"
end
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/works_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,8 @@ def load_owner
end
end
end
@owner = @pseud || @user || @collection || @tag
@language = Language.find_by(short: params[:language_id]) if params[:language_id].present?
@owner = @pseud || @user || @collection || @tag || @language
end

def load_work
Expand Down
2 changes: 2 additions & 0 deletions app/helpers/search_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ def search_header(collection, search, item_name, parent=nil)
header << ts("by %{byline}", byline: parent.byline)
when User
header << ts("by %{username}", username: parent.login)
when Language
header << ts("in %{language}", language: parent.name)
end

header << ts("in %{tag_link}", tag_link: link_to_tag_with_text(parent, parent.name)) if parent.is_a?(Tag)
Expand Down
1 change: 1 addition & 0 deletions app/models/language.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
class Language < ApplicationRecord
include WorksOwner
validates_presence_of :short
validates :short, uniqueness: true
validates_presence_of :name
Expand Down
6 changes: 6 additions & 0 deletions app/models/search/work_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ def queries

def add_owner
owner = options[:works_parent]

if owner.is_a?(Language)
options[:language_id] = owner.short
return
end

field = case owner
when Tag
:filter_ids
Expand Down
21 changes: 0 additions & 21 deletions app/views/languages/show.html.erb

This file was deleted.

3 changes: 2 additions & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -509,10 +509,11 @@
#### I18N ####

# should stay below the main works mapping
resources :languages do
resources :languages, except: [:show] do
resources :works
resources :admin_posts
end
get "/languages/:id", to: redirect("/languages/%{id}/works", status: 302)
Comment on lines -512 to +516
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more obvious that this route belongs to languages if we use the member block like

Suggested change
resources :languages do
resources :languages, except: [:show] do
resources :works
resources :admin_posts
end
get "/languages/:id", to: redirect("/languages/%{id}/works", status: 302)
resources :languages, except: [:show] do
resources :works
resources :admin_posts
member do
get :show, to: redirect("/languages/%{id}/works", status: 302)
end
end

Copy link
Contributor Author

@ceithir ceithir Nov 17, 2023

Choose a reason for hiding this comment

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

Doesn't appear to work. Or more exactly, it works too well, changing all get routes on /languages to a redirect.

I would say the get "/languages/:id", to:is good enough as is? It's consistent with official documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Oh no, definitely don't want that! I'm definitely good with your version over a version that breaks other things 😂

resources :locales, except: :destroy

#### API ####
Expand Down
3 changes: 2 additions & 1 deletion features/other_a/languages.feature
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,10 @@ Feature: Languages
# Browse works in a language

When I am on the languages page
And all indexing jobs have been run
Then I should see "Deutsch"
When I follow "Deutsch"
Then I should see "1 works in 1 fandoms"
Then I should see "1 Work in Deutsch"
And I should see "Die Rache der Sith"
And I should not see "Revenge of the Sith"

Expand Down
8 changes: 8 additions & 0 deletions features/step_definitions/work_steps.rb
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,14 @@
step "the periodic tag count task is run"
end

When "I browse works in language {string}" do |language_name|
step %{all indexing jobs have been run}
step "the periodic tag count task is run"

language = Language.find_by(name: language_name)
visit language_works_path(language)
end

When /^I delete the work "([^"]*)"$/ do |work|
work = Work.find_by(title: CGI.escapeHTML(work))
visit edit_work_path(work)
Expand Down
24 changes: 24 additions & 0 deletions features/works/work_browse.feature
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,27 @@ chapter when the chapters are reordered.
When I follow the comments link for the work "Awesome Work"
Then I should be on the work "Awesome Work"
And I should see "Bravo!"

Scenario: Can also browse work indexed by language
Given basic languages
And Persian language
And basic tags
And I am logged in
And I post the work "Whatever 1" with fandom "Aggressive Retsuko"
And I post the work "Whatever 2" with fandom "Aggressive Retsuko"
When I go to the new work page
And I select "Not Rated" from "Rating"
And I check "No Archive Warnings Apply"
And I fill in "Fandoms" with "Weiß Kreuz"
And I fill in "Work Title" with "Überraschende Überraschung"
And I fill in "content" with "Dies ist eine Fanfic in Deutsch."
And I select "Deutsch" from "Choose a language"
When I press "Post"
Then I should see "Work was successfully posted."
And I should see "Deutsch" within "dd.language"
When I browse works in language "English"
Then I should see "2 Works in English"
When I browse works in language "Deutsch"
Then I should see "1 Work in Deutsch"
When I browse works in language "Persian"
Then I should see "0 Works in Persian"
22 changes: 1 addition & 21 deletions spec/controllers/languages_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,26 +25,6 @@
end
end

describe "GET show" do
context "when not logged in" do
it "renders the show template" do
get :show, params: { id: "en" }
expect(response).to render_template("show")
end
end

Admin::VALID_ROLES.each do |role|
context "when logged in as an admin with #{role} role" do
let(:admin) { create(:admin, roles: [role]) }

it "renders the show template" do
get :show, params: { id: "en" }
expect(response).to render_template("show")
end
end
end
end

describe "GET new" do
context "when not logged in" do
it "redirects with error" do
Expand Down Expand Up @@ -227,7 +207,7 @@
end

it "redirects and returns success message" do
it_redirects_to_with_notice(finnish, "Language was successfully updated.")
it_redirects_to_with_notice(languages_path, "Language was successfully updated.")
end
end
end
Expand Down