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.

Sign up for GitHub

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

Jump to bottom

Improve validation for claims to be audited upload #1354

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Improve validation for claims to be audited upload #1354

wants to merge 8 commits into from

Conversation

dp-daly
Copy link
Contributor

@dp-daly dp-daly commented Jan 31, 2025 •
edited
Loading

Context

Improve validation for "claims to be audited" upload

Changes proposed in this pull request

  • Changes to Claims::UploadSamplingClaimsWizard and related steps to validate rows, headers and inputs of the uploaded CSV.

Guidance to review

  • Sign in as Colin (Support user)
  • Navigate to Claims -> Auditing (assuming you have claims in the paid status)
  • Click "Upload claims to be audited"
  • Upload a CSV with valid headers and inputs
  • You should see the CSV shown on the confirmation page
  • Click "Confirm upload"
  • You should see a success message
  • Next, trigger validation errors by uploading a CSV with missing or misspelt headers
  • Trigger an "invalid claim reference" validation error by uploading a CSV with a reference number which does not belong to a Claim with a status of "paid"
  • Trigger a "missing sample reason error" by uploading a CSV with a valid reference number which does not have a sample reason

Link to Trello card

https://trello.com/c/QTrfQQDW/377-csv-uploads-upload-claims-to-be-sampled

Screenshots

Screenshot 2025-01-31 at 17 36 04 Screenshot 2025-01-31 at 17 36 18 Screenshot 2025-01-31 at 17 38 22 Screenshot 2025-01-31 at 17 38 31

@dp-daly dp-daly requested review from a team as code owners January 31, 2025 17:39
@dp-daly dp-daly force-pushed the dpd/upload-sampling-claims-error-display branch from 11f8dc5 to 7eca88e Compare January 31, 2025 17:43
@dp-daly dp-daly self-assigned this Jan 31, 2025
@dp-daly dp-daly added the deploy A Review App will be created for PRs with this label label Feb 4, 2025
JamieCleare2525
JamieCleare2525 reviewed Feb 4, 2025
View reviewed changes
app/wizards/claims/upload_sampling_data_wizard/upload_step.rb
@@ -1,13 +1,18 @@
class Claims::UploadSamplingDataWizard::UploadStep < BaseStep
attribute :csv_upload
attribute :csv_content
attribute :file_name
attribute :claim_ids, default: []
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this attribute can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This relates to a question I've posed above: #1354 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is related to attribute :claim_ids, default: []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, claim_ids is currently used in process_csv but let me know if that should be different. This also highlighted to me that reset_claim_ids should be replaced by reset_input_attributes, so have updated that.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we no longer need to store the claim_ids, it can all be handled in the wizard itself. See the other wizards for how to do this.

@dp-daly dp-daly force-pushed the dpd/upload-sampling-claims-error-display branch from 7eca88e to 56e41be Compare February 4, 2025 13:07
@JamieCleare2525
Copy link
Contributor

https://www.figma.com/design/xWtG7TR9y1f9pvPMzOZ81a/Track-and-pay---OLD-DO-NOT-USE?node-id=61018-60471&t=dBgL3MUKncgmeJGk-0

image

There appear to be some missing text changes. The dropdown should need to go and add the inset text and header related guidance.

@dp-daly dp-daly force-pushed the dpd/upload-sampling-claims-error-display branch from 56e41be to 12a6fba Compare February 4, 2025 13:49
@dp-daly dp-daly force-pushed the dpd/upload-sampling-claims-error-display branch from 12a6fba to 458a00d Compare February 4, 2025 14:00
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 4, 2025

Deployments

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

@dp-daly dp-daly force-pushed the dpd/upload-sampling-claims-error-display branch from 458a00d to 17e39ee Compare February 4, 2025 16:11
JamieCleare2525
JamieCleare2525 reviewed Feb 5, 2025
View reviewed changes
app/views/wizards/claims/upload_sampling_data_wizard/_upload_errors_step.html.erb
</div>
</div>

<h2 class="govuk-heading-m">preview of <%= current_step.file_name %></h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

preview of should be in a locale.

JamieCleare2525
JamieCleare2525 reviewed Feb 5, 2025
View reviewed changes
app/views/wizards/claims/upload_sampling_data_wizard/_confirmation_step.html.erb
<% end %>

<% table.with_body do |body| %>
<% current_step.csv.each_with_index do |csv_row, index| %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<% current_step.csv.each_with_index do |csv_row, index| %>
<% current_step.csv.first(5).each_with_index do |csv_row, index| %>

This is something I missed. We only need to show the first 5.

JamieCleare2525
JamieCleare2525 reviewed Feb 5, 2025
View reviewed changes
app/views/wizards/claims/upload_sampling_data_wizard/_upload_step.html.erb
Comment on lines +15 to 17
<%= f.govuk_file_field :csv_upload, label: nil %>

<%= f.govuk_submit(t(".upload_csv_file")) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<%= f.govuk_file_field :csv_upload, label: nil %>
<%= f.govuk_submit(t(".upload_csv_file")) %>
<%= f.govuk_file_field :csv_upload, label: t(".upload_csv_file") %>
<%= f.govuk_submit(t(".upload")) %>

This input still needs the upload_csv_file label. It's the submit button which needs to change to just "Upload"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can make the label hidden.
But, I see what you mean about the changes in design. The text approach is fine.

JamieCleare2525
JamieCleare2525 reviewed Feb 5, 2025
View reviewed changes
...ims/sampling/upload_data/support_user_uploads_a_csv_containing_invalid_sampling_data_spec.rb
@@ -69,20 +58,24 @@ def when_i_click_on_upload_claims_to_be_sampled
end

def when_i_upload_a_csv_containing_invalid_sampling_data
attach_file "Upload CSV file", "spec/fixtures/claims/sampling/example_sampling_upload.csv"
attach_file nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is why you still need the upload_csv_file label on the file_input. You shouldn't be adding a file to nil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamieCleare2525 JamieCleare2525 JamieCleare2525 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@dp-daly dp-daly

Labels
deploy A Review App will be created for PRs with this label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dp-daly @JamieCleare2525