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

3343 partner profile #4051

Merged
merged 23 commits into from
Apr 25, 2024
Merged

Conversation

jadekstewart3
Copy link
Contributor

@jadekstewart3 jadekstewart3 commented Jan 22, 2024

Checklist:

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

Resolves #3343

Description

Moved program address (if different) fields to just below the agency information. This is currently completed for partners and tested to ensure partner profile is updating with new information in the new location. Banks still need to have this moved.

Todo

[X] Move program address (if different) for partners edit and show
[X] Test that agency selections are maintained during partner profile update
[X] Locate program address if different for banks, and move to the desired section just below the agency information
[X] Test to ensure partner profile updates as expected

Type of change

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

How Has This Been Tested?

Tested that edit functionality still works properly in:
spec/requests/profiles_requests_spec.rb
spec/requests/partners/profiles_requests_spec.rb

Screenshots

Banks:

Screenshot 2024-04-18 at 3 09 09 PM Screenshot 2024-04-18 at 3 35 26 PM

Partners:

Screenshot 2024-04-18 at 3 11 49 PM Screenshot 2024-04-18 at 3 11 14 PM

@cielf
Copy link
Collaborator

cielf commented Jan 22, 2024

Hey @jadekstewart3 -- a question rather than an actual review.... You are proposing a new partial -- Is the partial that this is currently in one of the ones that the organization controls the appearance of ? There is a field in the "My Organizations" for the bank around this. Just a sanity check.

@cielf
Copy link
Collaborator

cielf commented Feb 13, 2024

pinging @jadekstewart3 on this -- there was an open question.

@jadekstewart3
Copy link
Contributor Author

@cielf Thank you for pinging me, I put this ticket on the back burner until I get the kits sorted. If I understand your correctly, I was thinking of moving that field to a new partial, but I actually was unable to find the field under banks, only for the partners.. Does that answer your question?

@jadekstewart3 jadekstewart3 changed the title 3343 partner profile 3343 partner profile[WIP] Feb 14, 2024
@cielf
Copy link
Collaborator

cielf commented Feb 16, 2024

I looked into it -- the agency information partial is always shown -- several of the others are controlled by the bank choosing whether the partners see them or not. So the program address should also always be shown. In other words, there is nothing to be done re my question.

@jadekstewart3
Copy link
Contributor Author

@cielf So, if I'm understanding correctly, this ticket is no longer necessary? (Sorry, Im skeptical when there is less work to be done lol)

@cielf
Copy link
Collaborator

cielf commented Feb 16, 2024

@jadekstewart3 Ummm.. no - the moving of the fields is still necessary -- what isn't necessary is the extra work of making your new partial optional -- because the original partial is always shown, I'm assuming, at least for now, that we will always show this one.

@dorner
Copy link
Collaborator

dorner commented Apr 5, 2024

@jadekstewart3 is this still under development? There's been no activity in quite a while.

@jadekstewart3
Copy link
Contributor Author

Hey @dorner, I've been juggling a few things lately, but I'm ready to dive back into this task whenever you need me to. If someone else wants to give it a shot, I'm cool with that too. Just let me know! Thanks!

@dorner
Copy link
Collaborator

dorner commented Apr 11, 2024

@jadekstewart3 if you're good with picking it up again, please do!

@jadekstewart3
Copy link
Contributor Author

@dorner or @cielf In diaper_bank/manage/edit there currently are not fields for Program address (if different) if I am understanding correctly, because the Program address (if different ) fields are not currently present for the bank edit, would you like me to add that section in?

Or create an optional partial, to display if the current address fields have values? I remember @cielf saying that the optional partial was not necessary, but digging into it I am seeing a use case for it since it seems like we would be re using this section of code, so I just wanted to check :)

@cielf
Copy link
Collaborator

cielf commented Apr 17, 2024

I just edited a partner as org_admin1@example.com on staging, and there is a "Program / Delivery Address (if different)". I think we added in the Delivery bit after the issue was originated.

Oh. I see the issue -- diaper_bank/manage/edit would be editing the information for the diaper bank. We're talking about (for example) diaper_bank/profiles/2/edit -- which is editing the profile information for a partner.

@jadekstewart3 jadekstewart3 marked this pull request as ready for review April 18, 2024 21:41
@jadekstewart3 jadekstewart3 changed the title 3343 partner profile[WIP] 3343 partner profile Apr 18, 2024
@jadekstewart3
Copy link
Contributor Author

jadekstewart3 commented Apr 18, 2024

Screenshot 2024-04-18 at 4 11 17 PM

@cielf - I think I'm all finished with this, but rubocop is not passing CI for some reason? Locally its passing

@cielf
Copy link
Collaborator

cielf commented Apr 19, 2024

Yeah -- the CI also calls erblint, (the command in ci seems to be Run bundle exec erblint --lint-all) . This is catching a trailing newline in app/views/partners/profiles/edit/_agency_information.html.erb:58

@cielf
Copy link
Collaborator

cielf commented Apr 19, 2024

Hey @jadekstewart3 -- going ahead and kicking the tires on this -- I figure the erblint issue noted above will be exceedingly minor to fix.

Please check throughout that the heading for that group of fields is "Program / Delivery Address (if different)". I'm seeing a couple different variations of it. (I think we might have put the "Delivery" part of the label in after you started on this the first time.)

When viewing, the section should show whether or not the fields are filled in. (That's how it works on staging, and I don't know of a business reason to change it.)

Thanks!

@jadekstewart3
Copy link
Contributor Author

jadekstewart3 commented Apr 22, 2024

@cielf I think I have addressed what you've asked. I have an RSpec failure, but I suspect it may be due to flakey tests, and not my code. All checks are now passing :)

@cielf
Copy link
Collaborator

cielf commented Apr 22, 2024

I've got a block of work on HE slotted for tomorrow, will try to get to this then.

@cielf
Copy link
Collaborator

cielf commented Apr 23, 2024

LGTM -- @dorner.. Do you want to take a quick look for any technical concerns?

@cielf cielf requested a review from dorner April 23, 2024 15:56
@dorner
Copy link
Collaborator

dorner commented Apr 25, 2024

All good on my end!

@dorner dorner merged commit 82d1d91 into rubyforgood:main Apr 25, 2024
19 checks passed
Copy link
Contributor

@jadekstewart3: Your PR 3343 partner profile is part of today's Human Essentials production release: 2024.04.28.
Thank you very much for your contribution!

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.

Partner Profile: Move Program address (if different) to just below the agency information.
3 participants