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

RSpec to replicate duplicate JudgeAssignTasks problem #16382

Merged
merged 3 commits into from Jun 22, 2021

Conversation

yoomlam
Copy link
Contributor

@yoomlam yoomlam commented Jun 21, 2021

Bumps https://github.com/department-of-veterans-affairs/dsva-vacols/issues/174

Description

Add RSpec to replicate duplicate JudgeAssignTasks problem.

PR #16072 should address the problem.

Acceptance Criteria

  • Code compiles correctly

Testing Plan

Uncomment binding.pry in the RSpec and examine the browser tabs and task tree.
Check that json file contains no PII.

@yoomlam yoomlam requested a review from eileen-nava June 21, 2021 16:02
@yoomlam yoomlam self-assigned this Jun 21, 2021
@va-bot
Copy link
Collaborator

va-bot commented Jun 21, 2021

1 Warning
⚠️ This is a Big PR. Try to break this down if possible. Stacked pull requests encourage more detailed and thorough code reviews

Generated by 🚫 Danger

@codeclimate
Copy link

codeclimate bot commented Jun 21, 2021

Code Climate has analyzed commit 3d5612b and detected 0 issues on this pull request.

View more on Code Climate.

@yoomlam yoomlam requested a review from jcq June 21, 2021 20:21
Copy link
Contributor

@jcq jcq left a comment

Choose a reason for hiding this comment

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

This seems like a helpful test for showing current behavior (which allows multiple active JudgeAssignTasks, but shouldn't); this should help with verifying that #16072 works as intended.

@eileen-nava
Copy link
Contributor

Yoom, I looked at the task tree after activating the breakpoint on line 68, and I’m confused. I see one JudgeAssignTask with an open status, in_progress. The other JudgeAssignTask instances all have not-open statuses, cancelled or completed. Screenshot below:
Screen Shot 2021-06-21 at 5 29 47 PM

Should I examine the task tree at a different point in the test’s progress? Thanks!

@yoomlam
Copy link
Contributor Author

yoomlam commented Jun 21, 2021

Yoom, I looked at the task tree after activating the breakpoint on line 68, and I’m confused. I see one JudgeAssignTask with an open status, in_progress. The other JudgeAssignTask instances all have not-open statuses, cancelled or completed. Screenshot below:
...

Should I examine the task tree at a different point in the test’s progress? Thanks!

@eileen-nava Note the explain page was loaded before any modifications were made to the appeal to show the imported appeal. If you reload the explain page in your browser, it shows the most recent task tree. Or you can appeal.reload.treee in the pry prompt.

@yoomlam yoomlam merged commit 44618a8 into master Jun 22, 2021
@yoomlam yoomlam deleted the yoom/duplicate_jatasks branch June 22, 2021 12:45
Copy link
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

@eileen-nava Note the explain page was loaded before any modifications were made to the appeal to show the imported appeal. If you reload the explain page in your browser, it shows the most recent task tree. Or you can appeal.reload.treee in the pry prompt.

Got it, thanks! 👍

"middle_name": "P",
"name_suffix": null,
"closest_regional_office": null,
"ssn": "000765067",
Copy link
Contributor

Choose a reason for hiding this comment

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

In keeping with "Check that json file contains no PII," I want to confirm that none of the ssns in this file are actual social security numbers. (I don't think they are but, I am double-checking in the spirit of due diligence.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks. 👍

Functions.grant!("System Admin", users: ["PETERSBVAM"]) # enable access to `export` endpoint
end

# Ticket: https://github.com/department-of-veterans-affairs/dsva-vacols/issues/174
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it standard to link to tickets in comments? Part of me wonders if this could bloat the codebase over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean by bloat. The URL should always work. I like to include links to cross-references. In this case, the ticket provide more context in understanding the purpose of the RSpec and there may be updates to that ticket that aren't captured in this RSpec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, fine by me.

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.

None yet

4 participants