Implement partner groups #2131

albertchae · 2021-01-21T06:13:13Z

WIP, seeking feedback on UX, then will flesh out spec coverage

draft PR description, delete this and above once UX is settled

Resolves #2059
Partner PR: rubyforgood/partner#531

Description

Implement partner group tagging so diaper bank users can tag partners they work with into groups and also tag items that these partners can request.

I wasn't sure if we also want to enable tagging from the Inventory view and also the partner agencies view.

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

TODO:

Screenshots

TODO:

- migrations - factories - validation specs

TODO: add tests TODO: need to be able to add partners

change partners display in group Implement remove partner from partner group

- if the partner is not in any groups, return all items in organization - if the partner is a member in some groups, returns items tagged by those groups

armahillo · 2021-01-23T04:15:38Z

@albertchae

I had some suggestion about the naming: #2059 (comment) -- it would be a small lift to change the naming, but much easier to do it now before the migrations get merged in and deployed.
I'll defer to @edwinthinks since he's the boss now ( ;) )

Codewise, the PR looks solid; all the associations and forms look good, nice work.

edwinthinks · 2021-01-23T17:02:48Z

@albertchae @armahillo replied. I do agree with @armahillo that we probably don't mean PartnerGroup and I think we could change the name and the way we think about getting to the desired outcome.

Let's talk about in our coding session later :)

edwinthinks · 2021-05-08T19:55:50Z

@albertchae hey there! I've been giving this some thought and maybe a better place to start for this feature is figuring out how we can add ItemCategories and figure out if BaseItem is still something we want to require or keep? I've heard more use cases in which Organizations want to define their own Categories for Items so that in their reports it can be on Category not Item.

Let's talk about this :)!

…into 2059-partner-groups

edwinthinks · 2021-08-11T02:09:35Z

A few things left to do to keep track:

Include information on the partners#show about which group they are in and what items they can request
Be able to select which item categories PartnerGroups can request.

edwinthinks · 2021-08-28T14:14:02Z

Got this passing. I'll create a video going through the flow as I think its going to help figure out how this is all going to fit together.

edwinthinks · 2021-08-28T23:08:46Z

Here is a loom video explaining how to use it in practice - https://www.loom.com/share/ab271a1a9abe4e9da42030b5f67d9938. I'am aiming to send this to our stakeholders :)!

@albertchae can you please review this.

albertchae

Thanks again for taking over this PR and getting it to the finish line. Everything looks pretty good, I think my biggest question is about the data modeling where currently it seems a partner can only belong to a single partner group and whether that's sufficient.

albertchae · 2021-09-08T10:07:56Z

app/models/partner.rb


  belongs_to :organization
-  has_many :distributions, dependent: :destroy
+  belongs_to :partner_group, optional: true


does this mean a Partner can only belong to at most one PartnerGroup?

That is correct. The thought behind this is that keeping to a single association might be good enough for today's needs. If we discover it is lacking by complaints, then I think we can update it to include multiple associations.

sounds good

albertchae · 2021-09-08T10:09:06Z

app/controllers/partner_groups_controller.rb

+    @partner_group = current_organization.partner_groups.new(partner_group_params)
+    if @partner_group.save
+      # Redirect to groups tab in Partner page.
+      redirect_to partners_path + "#nav-partner-groups", notice: "Partner group added!"


I wonder if there's a better way to do this than string concatenating the # anchor, but we can save that for a future improvement

Yeh I wasn't too sure, how to achieve that without making another route.

albertchae · 2021-09-08T10:11:56Z

app/controllers/partners/children_controller.rb

+      @formatted_requestable_items = requestable_items.map do |rt|
+        [rt.name, rt.id]
+      end


should we add this as a format method somewhere? then we can reuse between new and edit. We could combine it with the call to PartnerFetchRequestableItemsService and have a fetch_and_format_requestable_items in this controller maybe?

albertchae · 2021-09-08T10:13:42Z

app/controllers/partners/individuals_requests_controller.rb

+      @formatted_requestable_items = requestable_items.map do |rt|
+        [rt.name, rt.id]
+      end


now that I'm seeing the same block here and other controllers, maybe we should have a "presenter" object to do the formatting. Arguably this duplication existed before this PR, so we could do it as a follow up too

Yeh I agree, I think some sort of mechanism to avoid duplication would be good! Will create an issue to handle that in another PR.

albertchae · 2021-09-08T10:15:54Z

app/controllers/partners_controller.rb

+    @partners = Partner.includes(:partner_group).where(organization: current_organization).class_filter(filter_params).alphabetized
+    @partner_groups = PartnerGroup.includes(:partners, :item_categories).where(organization: current_organization)


that's interesting we need this given the first query for partners. Could we instead do something like Partner.includes(partner_group: [:item_categories]) and remove @partner_groups? Then we could also go through partner to get the same thing, I think

Oh I see, we are using @partner_groups to give the user a list of groups they can attach a partner to. disregard my comment

app/models/partner.rb

edwinthinks · 2021-07-29T00:27:03Z

db/migrate/20201230233305_create_partner_groups.rb

+  def change
+    create_table :partner_groups do |t|
+      t.references :organization, foreign_key: true
+      t.string :name


I see that we added a compound uniqueness between organization_id and name via:

validates :name, presence: true, uniqueness: { scope: :organization }

I would add also a uniqueness index so that we add the constraint at the DB level.

edwinthinks · 2021-07-29T00:27:38Z

db/migrate/20210116200129_create_partner_group_items.rb

+class CreatePartnerGroupItems < ActiveRecord::Migration[6.0]
+  def change
+    create_table :partner_group_items do |t|
+      t.references :partner_group, foreign_key: true, null: false
+      t.references :item, foreign_key: true, null: false
+
+      t.timestamps
+    end
+  end
+end


Going to utilize a join table between PartnerGroup & ItemCategories that indicates which item categories are permitted to be requested.

edwinthinks · 2021-07-29T00:28:26Z

db/migrate/20201230233349_create_partner_group_memberships.rb

+    create_table :partner_group_memberships do |t|
+      t.references :partner_group, foreign_key: true
+      t.references :partner, foreign_key: true
+
+      t.timestamps
+    end


Giving this some thought -- I think support multiple memberships to different partner groups isn't needed yet. I'd be interested to make this a 1 to 1 relationship between Partner and PartnerGroup

edwinthinks · 2021-09-19T10:52:36Z

app/controllers/partner_groups_controller.rb

+    @partner_group = current_organization.partner_groups.new(partner_group_params)
+    if @partner_group.save
+      # Redirect to groups tab in Partner page.
+      redirect_to partners_path + "#nav-partner-groups", notice: "Partner group added!"


Yeh I wasn't too sure, how to achieve that without making another route.

edwinthinks · 2021-09-19T10:53:56Z

app/controllers/partners/individuals_requests_controller.rb

+      @formatted_requestable_items = requestable_items.map do |rt|
+        [rt.name, rt.id]
+      end


Yeh I agree, I think some sort of mechanism to avoid duplication would be good! Will create an issue to handle that in another PR.

edwinthinks · 2021-09-19T10:55:40Z

app/models/partner.rb


  belongs_to :organization
-  has_many :distributions, dependent: :destroy
+  belongs_to :partner_group, optional: true


That is correct. The thought behind this is that keeping to a single association might be good enough for today's needs. If we discover it is lacking by complaints, then I think we can update it to include multiple associations.

app/models/partner.rb

…into 2059-partner-groups

albertchae

LGTM, I actually can't approve it since I'm the original author but feel free to approve and merge @edwinthinks. Thanks for taking it over the finish line

SCDB · 2021-09-21T14:08:15Z

Hey there! Just chiming in to say most of our partner agencies are ordering from multiple programs/categories (diapers and period supplies, and in the future adult supplies as well). We unfortunately won't be able to use this functionality until we're able to assign one partner to multiple groups because reports and filtering results won't be complete if we try to use them before every partner can be properly categorized. Excited that this is so close!

edwinthinks · 2021-09-22T11:32:31Z

@SCDB thanks for the feedback :)! Would a workaround be to create PartnerGroups for each program/category combination?

SCDB · 2021-09-22T12:49:50Z

Yes!! Thanks for going where my brain could not. For now with only 2 programs that should be fairly simple. Once we add in the third program early next year, the volume of combinations might get a little much, but we can make that work for now.

SCDB · 2021-09-22T12:52:40Z

One other question--will this new functionality be tied to reporting or filtering options? One of the big reasons this feature could be so huge is the ability to save us tons of time counting up different purchases/donations/distributions for a particular program each month for our accountants. So excited!!

albertchae added 5 commits January 20, 2021 21:58

PartnerGroup and PartnerGroupMembership models

b4544cd

- migrations - factories - validation specs

Basic CRUD for PartnerGroups

bc90be3

TODO: add tests TODO: need to be able to add partners

Add partner to group via modal with dropdown

e433eeb

change partners display in group Implement remove partner from partner group

Implement tagging items with partner groups

cb18458

Implement PartnerRequestableItemsService

6f72ace

- if the partner is not in any groups, return all items in organization - if the partner is a member in some groups, returns items tagged by those groups

edwinthinks added 3 commits July 26, 2021 20:37

Merge branch 'main' into 2059-partner-groups

1e4abe0

Merge branch 'main' of https://github.com/rubyforgood/human-essentials …

7a2f9f3

…into 2059-partner-groups

Update migrations to utilize different data structure

07354ab

edwinthinks force-pushed the 2059-partner-groups branch from 1a7e8fe to 07354ab Compare July 29, 2021 00:46

Add associations and specs to verify them

fc68ff7

edwinthinks force-pushed the 2059-partner-groups branch from c3c973a to fc68ff7 Compare July 31, 2021 13:43

edwinthinks added 2 commits July 31, 2021 09:41

Update controller actions to fetch requestable items on partner frontend

d97bb89

Be able to associate Partner to PartnerGroup

492cc23

edwinthinks added 8 commits August 13, 2021 05:27

Get the tabs to render properly based on URL

c5929db

WIP

c0158d5

Got the functionality to work

f02f074

Add basic CRUD for partner group system specs

462b727

Add partner association to group spec

0f1194d

Update with main

52ef4bd

Remove unused code

200da94

Fix broken specs

7726fff

edwinthinks force-pushed the 2059-partner-groups branch 2 times, most recently from 678e55c to 8b291c7 Compare August 22, 2021 22:06

Fix rubocop complaints

e12f0ef

edwinthinks force-pushed the 2059-partner-groups branch from 8b291c7 to e12f0ef Compare August 23, 2021 11:52

edwinthinks changed the title ~~WIP: Implement partner groups~~ Implement partner groups Aug 25, 2021

edwinthinks added 2 commits August 25, 2021 17:46

Fix rubocop errors

a582437

Fix broken spec

4dec95f

edwinthinks assigned albertchae and edwinthinks Aug 28, 2021

Update

5a73dbd

Merge branch 'main' into 2059-partner-groups

b9c3574

albertchae commented Sep 8, 2021

View reviewed changes

edwinthinks reviewed Sep 19, 2021

View reviewed changes

edwinthinks added 2 commits September 19, 2021 05:56

Merge branch 'main' of https://github.com/rubyforgood/human-essentials …

902e51c

…into 2059-partner-groups

Update items association on partners to indicate purpose

88566c8

albertchae commented Sep 20, 2021

View reviewed changes

edwinthinks merged commit 407b128 into main Sep 21, 2021

seanmarcia deleted the 2059-partner-groups branch October 30, 2021 17:05

scooter-dangle mentioned this pull request Nov 3, 2021

sort item request dropdown #2602

Merged

		@partners = Partner.includes(:partner_group).where(organization: current_organization).class_filter(filter_params).alphabetized
		@partner_groups = PartnerGroup.includes(:partners, :item_categories).where(organization: current_organization)

Uh oh!

Implement partner groups #2131

Implement partner groups #2131

Uh oh!

Conversation

albertchae commented Jan 21, 2021

WIP, seeking feedback on UX, then will flesh out spec coverage

draft PR description, delete this and above once UX is settled

Description

Type of change

How Has This Been Tested?

Screenshots

Uh oh!

armahillo commented Jan 23, 2021

Uh oh!

edwinthinks commented Jan 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edwinthinks commented May 8, 2021

Uh oh!

edwinthinks commented Aug 11, 2021

Uh oh!

edwinthinks commented Aug 28, 2021

Uh oh!

edwinthinks commented Aug 28, 2021

Uh oh!

albertchae left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

albertchae left a comment

Choose a reason for hiding this comment

Uh oh!

SCDB commented Sep 21, 2021

Uh oh!

edwinthinks commented Sep 22, 2021

Uh oh!

SCDB commented Sep 22, 2021

Uh oh!

SCDB commented Sep 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

edwinthinks commented Jan 23, 2021 •

edited

Loading