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

Resolves #5000 #5107

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

Conversation

nolanborz
Copy link

@nolanborz nolanborz commented Mar 18, 2025

Checklist:

yes - I have performed a self-review of my own code,
yes - I have commented my code, particularly in hard-to-understand areas,
yes - I have made corresponding changes to the documentation,
yes - I have added tests that prove my fix is effective or that my feature works,
yes - New and existing unit tests pass locally with my changes ("bundle exec rake"),
yes - Title include "WIP" if work is in progress.
yes - I acknowledge that I will not force push my branch once reviews have started.

-->

Resolves #5000

Description

Hid the "My Co-Workers" item from the top-right dropdown for 'banks/org' users by adding an additional 'if' statement in the views/layouts/_lte_navbar.html.erb file to check if the user is a partner or not.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I added an rspec test which checks that "My Co-Workers" is not visible for org users after clicking on the top-right button. I added it to the most appropriate place I could find, spec/system/navigation_system_spec.rb. If we want to add more tests for the top-right dropdown menu we would probably add a new file, but it seemed out of the scope of this issue.

All tests passed.

Screenshots

mycoworker_visible_verifieduser org_user_mycoworker_notvisible

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Works fine! @awwaiid -- any technical adjustments needed?

@cielf cielf requested a review from awwaiid March 19, 2025 20:23
@@ -52,6 +52,18 @@
end
end
end
it "dropdown menu does not show My Co-Workers" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please change this into a request test? System tests are only needed for tests that require interactivity.

Also, please remove the puts statements.

Copy link
Author

Choose a reason for hiding this comment

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

For sure, I'll make those changes and get back to you.

Copy link
Author

Choose a reason for hiding this comment

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

Oh boy, I hope I did this right. The gits and commits and pushes and merges get me all jumbled up.
I ended up creating a new request testing file for the user dropdown. Actually remembered to run the lint this time too, which passed with 0 offenses. The only tests that failed are the tests that I think are failing for everyone at the moment? Not sure what's normal, but they seemed completely irrelevant to what I was working on.

@dorner
Copy link
Collaborator

dorner commented Mar 21, 2025

Lint is failing as well.

@dorner
Copy link
Collaborator

dorner commented Mar 27, 2025

Lint is still failing.

@nolanborz nolanborz force-pushed the 5000-remove-mycoworkers-from-org-user-dropdown branch from 5011963 to 6a179fc Compare March 27, 2025 02:39
@nolanborz
Copy link
Author

Please ignore the force. My most recent commit 96ae98e is the correct one. I sincerely apologize for the train wreck of a PR, cracked a little under the pressure trying to check and make sure I had done everything correctly.
The previous lint failed because I had failed to remove the system test which had an error lodged in it. Newest commit as a proper passing test in a new file for the user dropdown menu, and it actually passes the lints. If it's too messy and you want to start from scratch on the PR I'm completely up for it.

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.

Remove "My Co-workers" from top right corner menu for banks only
3 participants