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

[7784] Create review errors page #4822

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

Conversation

stevehook
Copy link
Contributor

@stevehook stevehook commented Nov 19, 2024

Context

After attempting to a bulk upload trainee CSV, if there are validation errors the whole upload fails so the user needs to be able to review their errors.

Changes proposed in this pull request

image

Guidance to review

Opening this up for comments. I will continue to do some manual tests.

Important business

  • Does this PR introduce any PII fields that need to be overwritten or deleted in db/scripts/sanitise.sql?
  • Does this PR change the database schema? If so, have you updated the config/analytics.yml file and considered whether you need to send 'import_entity' events?
  • Do we need to send any updates to DQT as part of the work in this PR?
  • Does this PR need an ADR?

NB: Please notify the #twd_data_insights team and ask for a review if new fields are being added to analytics.yml

@stevehook stevehook changed the title 7784 create review errors page [7784] Create review errors page Nov 19, 2024
@stevehook stevehook force-pushed the 7784-create-review-errors-page branch 5 times, most recently from 094184f to fb1c456 Compare November 20, 2024 10:31
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 94.33962% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.99%. Comparing base (ee46207) to head (8bedfa7).
Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
app/services/exports/bulk_trainee_upload_export.rb 66.66% 2 Missing ⚠️
...s/bulk_update/trainees/review_errors_controller.rb 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4822      +/-   ##
==========================================
- Coverage   95.99%   95.99%   -0.01%     
==========================================
  Files         726      729       +3     
  Lines       15795    15846      +51     
==========================================
+ Hits        15163    15211      +48     
- Misses        632      635       +3     
Flag Coverage Δ
unittests 95.99% <94.33%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@stevehook stevehook force-pushed the 7784-create-review-errors-page branch 2 times, most recently from 25bc733 to 7ee0e66 Compare November 23, 2024 19:53
spec/features/bulk_upload/bulk_add_trainees_spec.rb Outdated Show resolved Hide resolved
app/services/bulk_update/add_trainees/import_rows.rb Outdated Show resolved Hide resolved
app/services/bulk_update/add_trainees/import_rows.rb Outdated Show resolved Hide resolved
Comment on lines 168 to 203
scenario "when I try to upload a file with errors" do
when_i_visit_the_bulk_update_index_page
and_i_click_the_bulk_add_trainees_page
then_i_see_how_instructions_on_how_to_bulk_add_trainees
and_i_see_the_empty_csv_link

when_i_attach_a_file_with_invalid_rows
and_i_click_the_upload_button
when_the_background_job_is_run
and_i_refresh_the_page
then_i_see_the_review_page_with_no_errors

when_the_background_job_is_run
and_i_refresh_the_page
then_i_see_the_review_page_with_validation_errors

when_i_click_the_review_errors_link
then_i_see_the_review_errors_page

when_i_click_on_the_download_link
then_i_receive_the_file

when_i_return_to_the_review_errors_page
and_i_attach_a_valid_file
and_i_click_the_upload_button
then_i_see_that_the_upload_is_processing
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to refactor the the bulk add trainees page is visible scenario instead to avoid running similar assertions twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I ended up leaving this as a separate scenario was that it follows a different route through the flow. Because we can (re-)upload a corrected CSV through the review errors page, instead of going back to the normal page where we start the upload, it seemed like we would have to run two successful uploads to cover this in one scenario. Does that make sense? I am going to look at ways to reduce the duplication...

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is unavoidable to have a separate scenario no worries.

@stevehook stevehook force-pushed the 7784-create-review-errors-page branch from 97569a2 to 3080e6a Compare November 25, 2024 13:41
Copy link

sonarcloud bot commented Nov 25, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants