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

4515 - Deactivated partners should not appear in the dropdown when producing a new distribution. #4639

Merged
merged 6 commits into from
Sep 12, 2024

Conversation

ewoknock
Copy link
Contributor

@ewoknock ewoknock commented Sep 4, 2024

Resolves #4515

Description
When creating new distributions, the "Partner" dropdown should not contain deactivated partners.
However, when editing an existing distribution, you should still see deactivated partners.
This change is to reduce the number of errors and make it easier for the banks to create distributions.

To implement this feature, I added in a conditional check on the distribution form partial. If the "source" local variable has been passed to the partial and contains the value "new", then the deactivated partners will be filtered out of the partner list.
In any other case, the form partial will just render the full partner list associated with the bank.
This allows the bank to still edit distributions for partners that may have been deactivated since the distribution was created.

Type of change
New feature (non-breaking change which adds functionality)
How Has This Been Tested?
I first tested this manually by deactivated one of the partners.
I created a new distribution and confirmed that the partner was no longer in the dropdown.
I tried to make a new distribution but intentionally added in errors and confirmed that it would still properly filter out deactivated partners on a re-render.
I tried to edit some existing distributions and confirmed that the deactivated partners still showed up on the list.

I also created unit tests that are located in spec/system/distribution_system_spec.rb

Screenshots
New distribution after error filters out Pawnee Parent Service
image

Editing an existing distribution still allows you to select Pawnee Parent Service
image

This commit adds in a conditional check to the _form partial that
checks where the partial is coming from. If it is being rendered from
the 'new' view, it will then generate a partner list that filters out
the deactivated partners. If it is being rendered from anywhere else,
it will generate the complete partner list unfiltered.

[Ticket: 4515]
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.

LGTM from a functional pov

@cielf cielf requested a review from dorner September 6, 2024 14:09
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Love the assistance here! I had some suggestions on the approach.

@@ -1,3 +1,11 @@
<%
partner_list = if defined?(source) && source == 'new'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually shouldn't be calculated by the view at all. It's better to pass this in from whatever view is calling the partial.

Very simple DB queries are kind of OK within views, but in general we should be aiming not to have views talk to the DB at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense. I'll move the query into the controller actions and pass them from there.

create(:partner, name: 'Deactivated Partner', organization: organization, status: "deactivated")
end

it "should not display deactivated partners on new distribution" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one can be a request spec. It's faster than a system spec.

@ewoknock
Copy link
Contributor Author

ewoknock commented Sep 8, 2024

I've made those updates!

@dorner
Copy link
Collaborator

dorner commented Sep 12, 2024

Looks good now!

@dorner dorner merged commit 50e00e8 into rubyforgood:main Sep 12, 2024
12 checks passed
Copy link
Contributor

@ewoknock: Your PR 4515 - Deactivated partners should not appear in the dropdown when producing a new distribution. is part of today's Human Essentials production release: 2024.09.15.
Thank you very much for your contribution!

norrismei pushed a commit to norrismei/human-essentials that referenced this pull request Sep 22, 2024
…oducing a new distribution. (rubyforgood#4639)

* Filters out deactive partners on new distribution

This commit adds in a conditional check to the _form partial that
checks where the partial is coming from. If it is being rendered from
the 'new' view, it will then generate a partner list that filters out
the deactivated partners. If it is being rendered from anywhere else,
it will generate the complete partner list unfiltered.

[Ticket: 4515]

* Updates the re-render of create on a fail to also filter out the deactivated partners

* Adds in unit tests for distribution dropdown

* Removed unneeded variable declaration for unit tests

* Moves the partner_list query from the view into
the controller actions

* Moved rspec tests from 'system' to 'requests'
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.

Deactivated partners should not appear in the dropdown when producing a new distribution.
3 participants