Refactor edit claim to wizard pattern #1212

JamieCleare2525 · 2024-11-28T16:32:35Z

Context

To improve the efficiency of Editing a Claim. Refactor the Edit Claim journey to use the Wizard pattern.

Changes proposed in this pull request

Introduce EditClaimWizard and related steps
Remove code with is no longer necessary
Add test coverage to ensure all code changes have been tested.
... success?

Guidance to review

Sign in as Anne (School user)

Visit the show page for an existing DRAFT claim

Edit an existing claim (provider, mentors, mentor training hours)
Submit the claim -> The existing claim should be updated and submitted.

Click "Submit claim"
You should be redirected to the Declaration page.
Click "Submit claim" -> The existing clam should be submitted

Sign in as Colin (Support user)

Visit the show page for an existing DRAFT claim
Edit an existing claim (provider, mentors, mentor training hours)
Submit the claim -> The existing claim should be updated and submitted.

Link to Trello card

https://trello.com/c/dzMquW8D/934-refactor-the-edit-claim-form-to-use-the-wizard-pattern

github-actions · 2024-11-28T16:38:14Z

Deployments

App	URL
Track & Pay	https://track-and-pay-1212.test.teacherservices.cloud
School Placements	https://manage-school-placements-1212.test.teacherservices.cloud

Kizr · 2024-12-03T10:38:15Z

app/queries/claims/mentors_with_remaining_claimable_hours_query.rb

@@ -22,7 +22,7 @@ def remaining_claimable_hours_condition(scope)
  end

  def training_allowance_for(mentor)
-    Claims::TrainingAllowance.new(mentor:, provider:, academic_year:)
+    Claims::TrainingAllowance.new(mentor:, provider:, academic_year:, claim_to_exclude: claim)


What changed to require the addition of claim_to_exclude here?

Kizr · 2024-12-03T10:39:37Z

app/views/claims/support/claims/_details.html.erb

@@ -15,7 +15,8 @@
      <% row.with_value(text: claim.provider.name) %>
      <% if policy(claim).edit? %>
        <% row.with_action(text: t("change"),
-                           href: create_revision_claims_support_school_claim_path(claim.school, claim),
+                           href: edit_attribute_path(:provider),
+                           visually_hidden_text: Claims::Claim.human_attribute_name(:accredited_provider),


Nice spot on the visually_hidden_text arguments you've added ⭐

Kizr · 2024-12-03T10:42:29Z

app/views/wizards/claims/edit_claim_wizard/_declaration_step.html.erb

+        <% unless current_user.support_user? %>
+          <% summary_list.with_row do |row| %>
+            <% row.with_key(text: Claims::Claim.human_attribute_name(:school)) %>
+            <% row.with_value(text: @wizard.school.name) %>


Could we consider delegation here to guard against nil values? Granted this shouldn't happen but we've seen a lot of cases that shouldn't happen 😂

Likewise for the other chained methods in this file if you agree!

Kizr · 2024-12-03T10:47:26Z

app/wizards/claims/edit_claim_wizard.rb

+    end
+
+    def academic_year
+      claim.claim_window.academic_year


Could we make use of the has_one :academic_year, through: :claim_window association here?

Suggested change

claim.claim_window.academic_year

claim.academic_year

Is this worth memoizing?

Kizr · 2024-12-03T10:50:53Z

app/wizards/claims/edit_claim_wizard.rb

+        max_hours = Claims::TrainingAllowance.new(
+          mentor: mentor_training.mentor,
+          provider: mentor_training.provider,
+          academic_year: claim.claim_window.academic_year,


Could this make use of the academic_year method above?

Kizr · 2024-12-03T11:02:28Z

spec/system/claims/schools/claims/edit_draft_claim_spec.rb

@@ -70,7 +130,7 @@ def given_i_sign_in
  end

  def given_there_is_a_current_claim_window
-    Claims::ClaimWindow::Build.call(claim_window_params: { starts_on: 2.days.ago, ends_on: 2.days.from_now }).save!(validate: false)
+    claim_window


Given that we use let! to declare the claim_window is this step necessary?

Kizr · 2024-12-03T11:04:17Z

spec/system/claims/support/schools/claims/change_claim_on_check_page_spec.rb

@@ -216,21 +197,25 @@ def when_i_add_training_hours(hours)
  end

  def then_i_expect_the_training_hours_to_be_selected(hours)
-    find("#claims-support-claim-mentor-training-form-hours-completed-#{hours}-field").checked?
+    if hours.to_i == 20


Is it necessary to convert this to an integer here or could we use string comparison?

Kizr · 2024-12-03T11:05:07Z

spec/system/claims/support/schools/claims/change_claim_on_check_page_spec.rb

    expect(page).to have_content("Check your answers")

    within("dl.govuk-summary-list:nth(1)") do
      within(".govuk-summary-list__row:nth(1)") do
        expect(page).to have_content("Academic year")
-        expect(page).to have_content(claim.academic_year_name)
+        expect(page).to have_content(@claim_window.academic_year.name)


Similar to a previous comment 😁

Suggested change

expect(page).to have_content(@claim_window.academic_year.name)

expect(page).to have_content(@claim_window.academic_year_name)

Kizr · 2024-12-03T11:05:40Z

spec/system/claims/support/schools/claims/edit_draft_claim_spec.rb

-    given_i_sign_in
-    when_i_select_a_school
-    when_i_click_on_claims
+  scenario "A support user I can edit a draft claim" do


Suggested change

scenario "A support user I can edit a draft claim" do

scenario "As a support user I can edit a draft claim" do

Kizr · 2024-12-03T11:07:28Z

spec/wizards/claims/edit_claim_wizard_spec.rb

+  let(:school) { create(:claims_school) }
+  let(:created_by) { create(:claims_user, schools: [school]) }
+  let(:provider) { create(:claims_provider, :niot) }
+  let(:mentor_1) { create(:claims_mentor, schools: [school], first_name: "Alan", last_name: "Anderson") }


Suggested change

let(:school) { create(:claims_school) }

let(:created_by) { create(:claims_user, schools: [school]) }

let(:provider) { create(:claims_provider, :niot) }

let(:mentor_1) { create(:claims_mentor, schools: [school], first_name: "Alan", last_name: "Anderson") }

let(:school) { build(:claims_school) }

let(:created_by) { build(:claims_user, schools: [school]) }

let(:provider) { build(:claims_provider, :niot) }

let(:mentor_1) { build(:claims_mentor, schools: [school], first_name: "Alan", last_name: "Anderson") }

Jamie added 10 commits November 22, 2024 15:23

Introduce EditClaimWizard

5920cc8

Add declaration step

b01ed4f

School users use EditClaimWizard

d16fde6

Support User editing a claim uses wizard pattern

41f8988

Rspec passing

d683c18

Tests for EditClaimWizard

6a1e8e6

Undercover - No coverage is missing

0c65526

Refactor disclaimer components to work with claim wizards

335d1db

Fix argument change with disclaimer components

626429d

Remove unnecessary controllers and views

085604a

JamieCleare2525 added the deploy A Review App will be created for PRs with this label label Nov 28, 2024

JamieCleare2525 self-assigned this Nov 28, 2024

JamieCleare2525 requested review from a team as code owners November 28, 2024 16:32

JamieCleare2525 changed the base branch from main to jc/refactor_claims_to_wizard_pattern November 28, 2024 16:32

Jamie added 2 commits November 28, 2024 16:34

Add missing extra line to yaml

021b54e

Add missing extra line to yaml

b77f405

JamieCleare2525 temporarily deployed to review November 28, 2024 16:40 — with GitHub Actions Inactive

Kizr reviewed Dec 3, 2024

View reviewed changes

Kizr requested a review from Nitemaeric December 3, 2024 11:11

JamieCleare2525 had a problem deploying to review December 13, 2024 11:39 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor edit claim to wizard pattern #1212

Refactor edit claim to wizard pattern #1212

JamieCleare2525 commented Nov 28, 2024

github-actions bot commented Nov 28, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

Kizr Dec 3, 2024

	expect(page).to have_content(@claim_window.academic_year.name)
	expect(page).to have_content(@claim_window.academic_year_name)

	scenario "A support user I can edit a draft claim" do
	scenario "As a support user I can edit a draft claim" do

Refactor edit claim to wizard pattern #1212

Are you sure you want to change the base?

Refactor edit claim to wizard pattern #1212

Conversation

JamieCleare2525 commented Nov 28, 2024

Context

Changes proposed in this pull request

Guidance to review

Sign in as Anne (School user)

Sign in as Colin (Support user)

Link to Trello card

github-actions bot commented Nov 28, 2024

Deployments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment