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

3269 [BUGFIX] refactor reparse js file #3280

Merged
merged 5 commits into from
Nov 20, 2024

Conversation

raftmsohani
Copy link

@raftmsohani raftmsohani commented Nov 14, 2024

Summary of Changes

Pull request closes #3269

How to Test

  1. Open http://localhost:3000/ and sign in.
  2. Proceed to admin -> datafile
  3. You will need more than 500 datafiles to test the functionality. If you are testing this locally, you can create a loop using backend-exec-seed-db (you will need to change the script to use multiple STTs in a loop), otherwise test this in dev env
  4. If reparse command is not used, then form will not be submitted. Otherwise if 'reparse' command is chosen, then modal will show the number of files to be reparsed.

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

  • [insert ACs here]
  • lfrohlich and/or adpennington confirmed that ACs are met.

Deliverable 2: Tested Code

  • Are all areas of code introduced in this PR meaningfully tested?
    • If this PR introduces backend code changes, are they meaningfully tested?
    • If this PR introduces frontend code changes, are they meaningfully tested?
  • Are code coverage minimums met?
    • Frontend coverage: [insert coverage %] (see CodeCov Report comment in PR)
    • Backend coverage: [insert coverage %] (see CodeCov Report comment in PR)

Deliverable 3: Properly Styled Code

  • Are backend code style checks passing on CircleCI?
  • Are frontend code style checks passing on CircleCI?
  • Are code maintainability principles being followed?

Deliverable 4: Accessible

  • Does this PR complete the epic?
  • Are links included to any other gov-approved PRs associated with epic?
  • Does PR include documentation for Raft's a11y review?
  • Did automated and manual testing with iamjolly and ttran-hub using Accessibility Insights reveal any errors introduced in this PR?

Deliverable 5: Deployed

  • Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

  • Does this PR provide background for why coding decisions were made?
  • If this PR introduces backend code, is that code easy to understand and sufficiently documented, both inline and overall?
  • If this PR introduces frontend code, is that code easy to understand and sufficiently documented, both inline and overall?
  • If this PR introduces dependencies, are their licenses documented?
  • Can reviewer explain and take ownership of these elements presented in this code review?

Deliverable 7: Secure

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any new security issues?
  • If new issues detected, is investigation and/or remediation plan documented?

Deliverable 8: User Research

Research product(s) clearly articulate(s):

  • the purpose of the research
  • methods used to conduct the research
  • who participated in the research
  • what was tested and how
  • impact of research on TDP
  • (if applicable) final design mockups produced for TDP development

@raftmsohani raftmsohani self-assigned this Nov 14, 2024
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.48%. Comparing base (dd5ea65) to head (c2d142a).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3280   +/-   ##
========================================
  Coverage    91.48%   91.48%           
========================================
  Files          297      297           
  Lines         8433     8433           
  Branches       611      611           
========================================
  Hits          7715     7715           
  Misses         605      605           
  Partials       113      113           
Flag Coverage Δ
dev-backend 91.34% <ø> (ø)
dev-frontend 92.51% <ø> (ø)

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


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd5ea65...c2d142a. Read the comment docs.

---- 🚨 Try these New Features:

@raftmsohani raftmsohani changed the title 3269 refactor reparse js file 3269 [BUGFIX] refactor reparse js file Nov 14, 2024
@raftmsohani raftmsohani added the raft review This issue is ready for raft review label Nov 14, 2024
if (confirm("You are about to re-parse " + number_of_files.innerHTML.split(/(\s+)/)[0] + " files. Are you sure you want to continue?")) {
console.log('submitting');
theForm.submit();
for (var i = 0; i < theForm.childNodes.length; i++) {
Copy link

Choose a reason for hiding this comment

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

This works as advertised. But is there not a way for us to select the element containing the selected number of files instead of looping through children manually? Could we do something more like: document.querySelector("span.all");? I tried in the dev tools to see if it works and it returns the right element. We would just need to split on the space character to extract the number. We would also need to check that the element isn't hidden. If it was, then we just grab the other span.

Copy link

Choose a reason for hiding this comment

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

Forgot to include the screenshot:

Screenshot 2024-11-14 at 2 16 42 PM

Copy link

@elipe17 elipe17 left a comment

Choose a reason for hiding this comment

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

Left a suggestion. Let me know what you think.

Copy link

@elipe17 elipe17 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@andrew-jameson andrew-jameson left a comment

Choose a reason for hiding this comment

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

LGTM

@raftmsohani raftmsohani requested review from ADPennington and removed request for jtimpe November 18, 2024 19:00
@raftmsohani raftmsohani added QASP Review and removed raft review This issue is ready for raft review labels Nov 18, 2024
@ADPennington ADPennington added the Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI label Nov 18, 2024
Copy link
Collaborator

@ADPennington ADPennington left a comment

Choose a reason for hiding this comment

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

lgtm! 🚀

reparsegt100files
reparsegt100filesp2

@ADPennington ADPennington added Ready to Merge and removed QASP Review Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Nov 19, 2024
@raftmsohani raftmsohani merged commit 455ca37 into develop Nov 20, 2024
16 checks passed
@raftmsohani raftmsohani deleted the 3269-bug-reparse-action-confirmation-pop-up branch November 20, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] DAC reparse action confirmation pop-up does not include total # files selected if GT 100 files
4 participants