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

Code cleanup #264

Merged
merged 3 commits into from
Sep 22, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
31 changes: 0 additions & 31 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,37 +5,6 @@ class ApplicationController < ActionController::Base

before_action :check_service_open!

delegate :application_route, to: :current_application
helper_method :application_route

def check_application!
return if session["application_id"]

redirect_to(root_path)
end

def current_application
Application.in_progress.find(session["application_id"])
end
helper_method :current_application

def current_applicant
current_application.applicant
end
helper_method :current_applicant

def check_teacher!
return unless application_route != "teacher"

redirect_to(new_applicants_application_route_path)
end

def check_trainee!
return unless application_route != "salaried_trainee"

redirect_to(new_applicants_application_route_path)
end

def check_service_open!
return if request.path == destroy_user_session_path # skip this for log out page
return if Gatekeeper.application_open?
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/system_admin/applicants_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class ApplicantsController < AdminController
include Pagy::Backend

def index
results = Application.submitted
results = Application.all
.search(params[:search])
.filter_by_status(params[:status])
.order(created_at: :desc)
Expand Down
29 changes: 7 additions & 22 deletions app/models/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ class Application < ApplicationRecord
delegate :sla_breached?, to: :application_progress
delegate :status, to: :application_progress, allow_nil: false

default_scope { submitted }
scope :submitted, -> { where.not(urn: nil) }
scope :in_progress, -> { unscoped.where(urn: nil) }

scope :search, lambda { |term|
return if term.blank?

Expand Down Expand Up @@ -64,22 +60,11 @@ def mark_as_qa!
unique_by: %i[application_id status])
end

with_options if: :submitted? do
validates(:application_date, presence: true)
validates(:application_route, presence: true)
validates(:date_of_entry, presence: true)
validates(:start_date, presence: true)
validates(:subject, presence: true)
validates(:visa_type, presence: true)
validates(:urn, presence: true)
validates(:applicant, presence: true)
end

def assign_urn!
update!(urn: Urn.generate(application_route))
end

def submitted?
urn.present?
end
validates(:application_date, presence: true)
validates(:application_route, presence: true)
validates(:date_of_entry, presence: true)
validates(:start_date, presence: true)
validates(:subject, presence: true)
validates(:visa_type, presence: true)
validates(:applicant, presence: true)
end
2 changes: 1 addition & 1 deletion app/models/kpis.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def initialize
end

def total_applications
Application.submitted.count
Application.count
end

def total_rejections
Expand Down
2 changes: 1 addition & 1 deletion app/queries/average_age_query.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class AverageAgeQuery
def initialize(relation = Applicant.all)
@relation = relation.joins(:application).merge(Application.submitted)
@relation = relation.joins(:application).merge(Application.all)
end

def call
Expand Down
2 changes: 1 addition & 1 deletion app/queries/gender_breakdown_query.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class GenderBreakdownQuery
def initialize(relation = Applicant.all)
@relation = relation.joins(:application).merge(Application.submitted)
@relation = relation.joins(:application).merge(Application.all)
end

def call
Expand Down
2 changes: 1 addition & 1 deletion app/queries/nationality_breakdown_query.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class NationalityBreakdownQuery
def initialize(relation = Applicant.all)
@relation = relation.joins(:application).merge(Application.submitted)
@relation = relation.joins(:application).merge(Application.all)
end

def call
Expand Down
2 changes: 1 addition & 1 deletion app/queries/rejection_reason_breakdown_query.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class RejectionReasonBreakdownQuery
def initialize(relation = ApplicationProgress.all)
@relation = relation.joins(:application).merge(Application.submitted).where.not(rejection_reason: nil)
@relation = relation.joins(:application).merge(Application.all).where.not(rejection_reason: nil)
end

def call
Expand Down
2 changes: 1 addition & 1 deletion app/queries/route_breakdown_query.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class RouteBreakdownQuery
def initialize(relation = Application.all)
@relation = relation.submitted
@relation = relation
end

def call
Expand Down
2 changes: 1 addition & 1 deletion app/queries/subject_breakdown_query.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class SubjectBreakdownQuery
def initialize(relation = Application.all)
@relation = relation.submitted
@relation = relation
end

def call
Expand Down
2 changes: 1 addition & 1 deletion app/queries/visa_breakdown_query.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class VisaBreakdownQuery
def initialize(relation = Application.all)
@relation = relation.submitted
@relation = relation
end

def call
Expand Down
22 changes: 11 additions & 11 deletions app/services/submit_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@ def submit_form!

def create_application_records
ActiveRecord::Base.transaction do
school = create_school
applicant = create_applicant(school)
@application = create_application(applicant)
create_school
create_applicant
create_application
delete_form
end
end

def create_school
School.create!(
@school = School.create!(
name: form.school_name,
headteacher_name: form.school_headteacher_name,
address_attributes: {
Expand All @@ -56,8 +56,8 @@ def create_school
)
end

def create_applicant(school)
Applicant.create!(
def create_applicant
@applicant = Applicant.create!(
ip_address: ip_address,
given_name: form.given_name,
middle_name: form.middle_name,
Expand All @@ -75,13 +75,13 @@ def create_applicant(school)
city: form.city,
postcode: form.postcode,
},
school: school,
school: @school,
)
end

def create_application(applicant)
Application.create!(
applicant: applicant,
def create_application
@application = Application.create!(
applicant: @applicant,
application_date: Date.current.to_s,
application_route: form.application_route,
application_progress: ApplicationProgress.new,
Expand All @@ -100,7 +100,7 @@ def delete_form
def send_applicant_email
GovukNotifyMailer
.with(
email: application.applicant.email_address,
email: @applicant.email_address,
urn: application.urn,
)
.application_submission
Expand Down
10 changes: 1 addition & 9 deletions spec/factories/applications.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
visa_type { VisaStep::VALID_ANSWERS_OPTIONS.reject { _1 == "Other" }.sample }
date_of_entry { Time.zone.today }
start_date { 1.month.from_now.to_date }
submitted
urn { Urn.generate(application_route) }

factory :teacher_application do
application_route { "teacher" }
Expand All @@ -45,14 +45,6 @@
applicant strategy: :create, factory: :applicant
end

trait :submitted do
urn { Urn.generate(application_route) }
end

trait :not_submitted do
urn { nil }
end

trait :with_initial_checks_completed do
application_progress strategy: :build, factory: %i[application_progress initial_checks_completed]
end
Expand Down
6 changes: 3 additions & 3 deletions spec/features/admin_console/applications_list_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,13 @@
def given_there_are_few_applications
# Create 2 specific applications for search tests
unique_applicant = create(:applicant, given_name: "Unique Given Name", middle_name: "Unique Middle Name", family_name: "Unique Family Name", email_address: "[email protected]")
create(:application, :submitted, applicant: unique_applicant, urn: "Unique Urn 1")
create(:application, applicant: unique_applicant, urn: "Unique Urn 1")

another_applicant = create(:applicant, given_name: "Another Given Name", middle_name: "Another Middle Name", family_name: "Another Family Name", email_address: "[email protected]")
create(:application, :submitted, applicant: another_applicant, urn: "Unique Urn 2")
create(:application, applicant: another_applicant, urn: "Unique Urn 2")

# Create 19 more applications for pagination test
create_list(:application, 19, :submitted)
create_list(:application, 19)
end

def given_there_is_an_application_that_breached_sla
Expand Down
2 changes: 1 addition & 1 deletion spec/features/admin_console/download_qa_csv_file_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
end

def given_there_are_few_applications
create_list(:application, 5, :submitted)
create_list(:application, 5)
end

def when_i_am_in_the_applications_list_page
Expand Down
30 changes: 4 additions & 26 deletions spec/features/admin_console/duplicates_search_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
let!(:applicant_four) { build(:applicant, email_address: "[email protected]", passport_number: "123456", phone_number: "999999999") }

before do
create(:application, :submitted, applicant: applicant_one)
create(:application, :submitted, applicant: applicant_two)
create(:application, :submitted, applicant: applicant_three)
create(:application, :submitted, applicant: applicant_four)
create(:application, applicant: applicant_one)
create(:application, applicant: applicant_two)
create(:application, applicant: applicant_three)
create(:application, applicant: applicant_four)
end

it "Admin can search for duplicates by email" do
Expand All @@ -34,20 +34,6 @@
then_i_see_matching_duplicates_by_passport_number
end

it "the view renders even where there are 'in progress' applications" do
given_i_am_signed_as_an_admin
when_there_are_in_progress_applications
when_i_visit_the_duplicates_page
then_i_see_the_in_progress_applications
end

it "shows duplicates only once even if they match multiple times" do
given_i_am_signed_as_an_admin
when_there_are_in_progress_applications
when_i_visit_the_duplicates_page
then_i_see_the_in_progress_applications_only_once
end

def when_i_search_for_a_duplicate_by(type)
visit duplicates_path
case type
Expand All @@ -60,18 +46,10 @@ def when_i_search_for_a_duplicate_by(type)
end
end

def when_there_are_in_progress_applications
create(:application, applicant: nil, urn: nil, application_progress: nil)
end

def when_i_visit_the_duplicates_page
visit duplicates_path
end

def then_i_see_the_in_progress_applications
expect(page).to have_content("Duplicated applications")
end

def then_i_see_matching_duplicates
expect(page).to have_content("[email protected]")
expect(page).to have_content("123456")
Expand Down
39 changes: 1 addition & 38 deletions spec/models/application_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
RSpec.describe Application do
describe "validations" do
context "when the application has been submitted" do
subject(:application) { build(:teacher_application, :submitted) }
subject(:application) { build(:teacher_application) }

it { expect(application).to validate_presence_of(:application_date) }
it { expect(application).to validate_presence_of(:application_route) }
Expand All @@ -36,30 +36,9 @@
it { expect(application).to validate_presence_of(:visa_type) }
it { expect(application).to validate_presence_of(:applicant) }
end

context "when the application has not been submitted" do
it "is valid" do
expect(described_class.new).to be_valid
end
end
end

describe "scopes" do
describe ".submitted" do
it "returns applications with a URN" do
create_list(:application, 2, :submitted)
create(:application, :not_submitted)

expect(described_class.submitted.count).to eq 2
end

it "does not return applications without a URN" do
create(:application, :not_submitted)

expect(described_class.submitted.count).to eq 0
end
end

describe ".filter_by_status" do
it "returns applications with the specified status" do
initial_checks_application = build(:application)
Expand Down Expand Up @@ -95,22 +74,6 @@
end
end

describe "#assign_urn!" do
it "matches the required format for a teacher" do
application = create(:teacher_application)
application.assign_urn!

expect(application.reload.urn).to match(/^IRPTE[A-Z0-9]{5}$/)
end

it "matches the required format for a trainee" do
application = create(:salaried_trainee_application)
application.assign_urn!

expect(application.reload.urn).to match(/^IRPST[A-Z0-9]{5}$/)
end
end

describe ".search" do
let(:applicant_john) { create(:applicant, given_name: "John", family_name: "Doe", email_address: "[email protected]", passport_number: "123456") }
let(:application_john) { create(:application, urn: "APP123", applicant: applicant_john) }
Expand Down
1 change: 0 additions & 1 deletion spec/models/kpis_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
RSpec.describe Kpis do
describe "#total_applications" do
it "returns the total number of applications" do
create(:application, :not_submitted)
create_list(:application, 5)

kpis = described_class.new
Expand Down
2 changes: 0 additions & 2 deletions spec/queries/average_age_query_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
describe "#call" do
context "when there are a few applicants" do
before do
create(:application, :not_submitted, applicant: create(:applicant, date_of_birth: 25.years.ago))

create(:application, applicant: create(:applicant, date_of_birth: 35.years.ago))
create(:application, applicant: create(:applicant, date_of_birth: 45.years.ago))
create(:application, applicant: create(:applicant, date_of_birth: 52.years.ago))
Expand Down
Loading