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

Hide hearing worksheet and details links from VSO users #9463

Merged
merged 11 commits into from
Feb 18, 2019

Conversation

tomas-nava
Copy link
Contributor

Resolves #8793

Description

Hides the Hearing Worksheet and Hearing Details links from VSO users.

before:
screen shot 2019-02-15 at 3 01 39 pm

after:
screen shot 2019-02-15 at 2 58 54 pm

Copy link
Contributor

@joeyyang joeyyang left a comment

Choose a reason for hiding this comment

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

LGTM! A few small comments that would be nice to have, but I'll defer to you on these.

let(:attorney_first_name) { "Robby" }
let(:attorney_last_name) { "McDobby" }
let(:attorney_first_name) { "Chanel" }
let(:attorney_last_name) { "Afshari" }
Copy link
Contributor

Choose a reason for hiding this comment

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

excellent names here

scroll_element_in_to_view("#hearings-section")
expect(page).to_not have_content("View VLJ Hearing Worksheet")
expect(page).to_not have_css("a[href='/hearings/#{hearing.external_id}/worksheet/print?keep_open=true']")
expect(page).to_not have_content("View Hearing Details")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd replace these strings here with their equivalents from COPY.json: COPY::CASE_DETAILS_HEARING_DETAILS_LINK_COPY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with this commit.


expect(page).to have_current_path("/queue/appeals/#{appeal.vacols_id}")
scroll_element_in_to_view("#hearings-section")
expect(page).to_not have_content("View VLJ Hearing Worksheet")
Copy link
Contributor

Choose a reason for hiding this comment

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

COPY::CASE_DETAILS_HEARING_WORKSHEET_LINK_COPY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with this commit.

</Link>
}];

if (!userIsVsoEmployee) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine to me, but I think the method would be much more readable if it follows one path if userIsVsoEmployee and another path if !userIsVsoEmployee, so something like:

let hearingAttrs;

if (userIsVsoEmployee) {
  hearingAttrs = [{
     ...
  ]};
} else ( 
  hearingAttrs = [{
    ...
  }];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hear you, but there are multiple conditions being checked to compose this list (the values of userIsVsoEmployee, hearing.disposition, and hearing.viewedByJudge all affect how the "Disposition" element is rendered) which means there are more than two paths.

@tomas-nava tomas-nava added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Feb 15, 2019
@ghost ghost assigned va-bot Feb 16, 2019
@va-bot va-bot merged commit b18e57e into master Feb 18, 2019
@va-bot va-bot deleted the queue/tomas/8793-hide-hearing-worksheet-link-from-vsos branch February 18, 2019 20:15
@ghost ghost removed the In-Progress label Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants