Configure bank organization units #4432

awwaiid · 2024-06-09T02:30:12Z

This PR builds on #4415 and works toward #4396.

This adds partner-request-units configuration for Bank Organizations.

…om-request-units-at-organizational-level Conflicts: app/controllers/organizations_controller.rb

…anizational-level

cielf

I think the dropdown with the units they have used is not useful, and is potentially confusing. Can we get rid of it, please?
I also notice that if you enter a unit that is a substring of an already entered unit, it won't take it (though it will accept a superstring of an already entered unit) Do I think that's going to be a problem? Probably not.

awwaiid · 2024-06-11T17:57:54Z

Thank you for looking -- I'll give it a try to get rid of the extra drop-down when in this tags mode, but I had some issue with it. I might have to go back to the more hand-built UI if I can't get it working well enough.

…

On Tue, Jun 11, 2024 at 1:02 PM CL Fisher ***@***.***> wrote: ***@***.**** requested changes on this pull request. I think the dropdown with the units they have used is not useful, and is potentially confusing. Can we get rid of it, please? I also notice that if you enter a unit that is a substring of an already entered unit, it won't take it (though it will accept a superstring of an already entered unit) Do I think that's going to be a problem? Probably not. — Reply to this email directly, view it on GitHub <#4432 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACDQWMH5ZJACPD67TJTBTZG4URTAVCNFSM6AAAAABJAPIJMKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMJRGAYDQNJRGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

cielf · 2024-06-19T17:00:24Z

So this is back to the more manual way, then. That seems to work.

cielf

Back to the old way. Seems to work, but when I look at the code, it looks like we're committing a lot of unrelated stuff. I presume in error?

awwaiid · 2024-06-19T21:41:05Z

@cielf Try again, I switched the target base branch for this PR. Diff is much smaller :)

awwaiid · 2024-06-19T21:42:33Z

app/javascript/controllers/select2_controller.js

@@ -5,17 +5,29 @@ import "select2"
 export default class extends Controller {
  static values = {
    config: { type: Object, default: {} },
-  }
+    hideDropdown: { type: Boolean, default: false }


Changes in this file are:

Allow for more than one select2 on the same page (this was previously broken)

Add a new parameter that can hide the dropdown (for the 'tags' use-case)

awwaiid · 2024-06-19T21:44:18Z

app/views/organizations/edit.html.erb

+
+                <% if Flipper.enabled?(:enable_packs) %>
+                  <%= label_tag "organization[request_unit_names]", 'Custom request units used (please use singular form -- e.g. pack, not packs)' %>
+                  <%= select_tag(


This whole block is kinda ugly and I welcome feedback. I did a raw select tag so that I could give it a field name that isn't part of the model -- request_unit_names. It also uses the name as both the value and the display text. The rest is configuration for the select2.

Would we reuse this type of UI anywhere else? Would it make sense to put it e.g. in ui_helpers?

Maybe but I think not worth it until that happens. I might be biased since I wanna be done with this PR :)

cielf

Works nicely. Over to @dorner for any technical comments.

Better now. I don' t think we need to be overly concerned about the substring issue -- there is an obvious workaround if anyone does run into it.

dorner · 2024-06-21T19:06:00Z

app/services/organization_update_service.rb

+        request_unit_ids = params[:request_unit_names].reject(&:blank?).map do |request_unit_name|
+          Unit.find_or_create_by(organization: organization, name: request_unit_name).id
+        end
+        params.delete(:request_unit_names)


Not the biggest fan of manipulating the params hash unless it's cloned first. You get confusing logs etc.

OK -- I switched to a duplicate and avoided mutating the original. I used dup instead of deep_dup so that it would maintain the params require/permit logic. I think.

dorner · 2024-06-21T19:06:57Z

app/views/organizations/edit.html.erb

+
+                <% if Flipper.enabled?(:enable_packs) %>
+                  <%= label_tag "organization[request_unit_names]", 'Custom request units used (please use singular form -- e.g. pack, not packs)' %>
+                  <%= select_tag(


Would we reuse this type of UI anywhere else? Would it make sense to put it e.g. in ui_helpers?

dorner · 2024-06-21T19:08:23Z

spec/models/unit_spec.rb

+#  updated_at      :datetime         not null
+#  organization_id :bigint
+#
+require "rails_helper"


Isn't this auto-required?

Yes, though it's required in tons of these specs. I'm removing it from here and separately we can bulk-remove it.

I created #4529 to get the others

dorner · 2024-06-21T19:08:51Z

spec/requests/organization_requests_spec.rb

@@ -22,6 +23,14 @@
          url: organization.url
        )
      end
+
+      context "when enable_packs flipper is on" do


Should we validate that it does not display it when the flag is off?

dorner · 2024-06-21T19:08:57Z

spec/requests/organization_requests_spec.rb

@@ -58,6 +67,15 @@
                                            url: organization.url
                                          )
      end
+
+      context "when enable_packs flipper is on" do


dorner · 2024-06-21T19:09:23Z

spec/system/organization_system_spec.rb

+        expect(page).to_not have_content("Custom Request Units")
+      end
+
+      it "Shows custom request units when enabled", :aggregate_failures do


Can this be converted into a request spec?

Looks like the system spec is still here :)

OK updated now -- I moved almost all of the system specs over to the request spec, leaving only two that have pop-up interactions.

Note that several of these tests I moved basically as-is though they are flawed -- like they are looking for the word "Yes" on the page as their success, but all the other fields could have "Yes" visible. Unchanged validation we should address separately.

dorner · 2024-07-11T20:53:35Z

spec/system/organization_system_spec.rb

+        expect(page).to_not have_content("Custom Request Units")
+      end
+
+      it "Shows custom request units when enabled", :aggregate_failures do


Looks like the system spec is still here :)

dorner · 2024-07-11T20:53:53Z

spec/requests/organization_requests_spec.rb

+          Flipper.disable(:enable_packs)
+          get edit_organization_path
+          expect(response.body).to_not include("Custom request units used (please use singular form -- e.g. pack, not packs)")
+          expect(response.body).to_not include unit.name


Can we specify the unit name rather than relying on generated data?

dorner · 2024-07-16T20:04:46Z

Nice! Excited to get this in!

github-actions · 2024-07-21T14:43:12Z

@awwaiid: Your PR Configure bank organization units is part of today's Human Essentials production release: 2024.07.21.
Thank you very much for your contribution!

Eccatoe and others added 11 commits May 31, 2024 18:09

WIP Added Custom request units at ogranizational level

3a2ed8b

Add unique validation to custom organization unit name

ba7d8dd

Added flipper conditionals and specs

c299fa6

Fixed the button name and delete issue

f9b13f7

Updated to allow organizations to delete units

506392b

Clean out some lint!

58ca320

Merge remote-tracking branch 'origin/main' into 4396-PACKS-1-add-cust…

67f88f3

…om-request-units-at-organizational-level Conflicts: app/controllers/organizations_controller.rb

Merge branch 'main' into 4396-PACKS-1-add-custom-request-units-at-org…

94b9360

…anizational-level

Use select2 in tags-mode for Bank unit configuration

c2319ac

Modify select2 to allow multiple instances on the same page

35c2d2f

Update spec to match request units param

85536e2

cielf self-requested a review June 11, 2024 16:55

cielf previously requested changes Jun 11, 2024

View reviewed changes

awwaiid added 3 commits June 19, 2024 11:09

Merge remote-tracking branch 'origin/main' into org-units-edit

a31f3cd

Merge remote-tracking branch 'origin/main' into org-units-edit

4300a0d

Hide the select2 drop-down for units [#4396]

db4862a

cielf reviewed Jun 19, 2024

View reviewed changes

awwaiid added 2 commits June 19, 2024 16:35

Use unit-names instead of id's when configuring bank [#4396]

294d8ab

Update some org-unit config specs [#4396]

5d9bf63

awwaiid changed the base branch from 4396-PACKS-1-add-custom-request-units-at-organizational-level to main June 19, 2024 21:38

awwaiid requested a review from cielf June 19, 2024 21:41

awwaiid commented Jun 19, 2024

View reviewed changes

Remove outdated comment [#4396]

cb798c8

awwaiid commented Jun 19, 2024

View reviewed changes

cielf reviewed Jun 20, 2024

View reviewed changes

cielf requested a review from dorner June 20, 2024 20:44

dorner requested changes Jun 21, 2024

View reviewed changes

awwaiid changed the title ~~Configure bank organization units using select2~~ Configure bank organization units Jun 22, 2024

awwaiid mentioned this pull request Jun 22, 2024

4396 packs 1 add custom request units at organizational level #4415

Closed

awwaiid added 4 commits June 22, 2024 14:11

Use a dup of params and don't mutate the original [#4396]

e80c9ae

Merge remote-tracking branch 'origin/main' into org-units-edit

0aeb851

Remove implicit rails_helper require [#4396]

60560b7

Explicitly check for packs-disabled case [#4396]

fd396ca

dorner requested changes Jul 11, 2024

View reviewed changes

awwaiid added 3 commits July 13, 2024 20:59

Merge remote-tracking branch 'origin/main' into org-units-edit

a79d052

Move most organization system specs to request spec [#4396]

b5d12cd

Clean out some lint [#4396]

e30a775

awwaiid requested a review from dorner July 14, 2024 18:55

Switch two more unit-name validation consts [#4396]

05887cf

dorner approved these changes Jul 16, 2024

View reviewed changes

dorner merged commit d4c64a0 into main Jul 16, 2024
38 checks passed

dorner deleted the org-units-edit branch July 16, 2024 20:04

awwaiid added this to the Request Units (Packs) milestone Jul 21, 2024

awwaiid linked an issue Jul 21, 2024 that may be closed by this pull request

[PACKS] # 1 Add custom request units at organizational level #4396

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure bank organization units #4432

Configure bank organization units #4432

awwaiid commented Jun 9, 2024 •

edited

Loading

cielf left a comment

awwaiid commented Jun 11, 2024 via email

cielf commented Jun 19, 2024

cielf left a comment

awwaiid commented Jun 19, 2024

awwaiid Jun 19, 2024

awwaiid Jun 19, 2024

dorner Jun 21, 2024

awwaiid Jul 14, 2024

cielf left a comment

dorner Jun 21, 2024

awwaiid Jun 22, 2024

dorner Jun 21, 2024

dorner Jun 21, 2024

awwaiid Jul 7, 2024

awwaiid Jul 14, 2024

dorner Jun 21, 2024

awwaiid Jul 7, 2024

dorner Jun 21, 2024

awwaiid Jul 7, 2024

dorner Jun 21, 2024

dorner Jul 11, 2024

awwaiid Jul 14, 2024

awwaiid Jul 14, 2024

dorner Jul 11, 2024

dorner Jul 11, 2024

awwaiid Jul 14, 2024

dorner commented Jul 16, 2024

github-actions bot commented Jul 21, 2024

Configure bank organization units #4432

Configure bank organization units #4432

Conversation

awwaiid commented Jun 9, 2024 • edited Loading

cielf left a comment

Choose a reason for hiding this comment

awwaiid commented Jun 11, 2024 via email

cielf commented Jun 19, 2024

cielf left a comment

Choose a reason for hiding this comment

awwaiid commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cielf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner commented Jul 16, 2024

github-actions bot commented Jul 21, 2024

awwaiid commented Jun 9, 2024 •

edited

Loading