Add owner grouping logic to add/remove owners #1059

lukepag · 2025-02-26T05:03:21Z

Ticket Link

List of squashed commits

Add owner grouping logic to add/remove owners
add new modals for adding and removing group path owners
Create new controller functions
Implement new grouping owner add/remove logic
Fixed add bug
Implement hyperlink for grouping owners
Add more comments and fix CSS spacing
Create and update existing jasmine tests
Finalize clean up

Test Checklist

Exhibits Clear Code Structure:
Project Unit Tests Passed:
Project Jasmine Tests Passed:
Executes Expected Functionality:
Tested For Incorrect/Unexpected Inputs:

mhodgesatuh

 * Give ownership of grouping at grouping path to newOwner. A user with owner privileges has
 * read and write privileges
 * of a grouping.
 
 The above should be on two lines.
 
 In the comments the spelling "owners-group" might be clearer.

JorWo · 2025-02-26T19:52:05Z

src/main/resources/static/javascript/mainApp/grouping.controller.js

+                        if(owner.name.includes(':')) {
+                            owner.isOwnerGrouping = true;
+                            // Name field in owner groupings initially holds the group path
+                            owner.ownerGroupingPath = owner.name;
+                            const splitOwnerPath = owner.name.split(':');
+                            // Isolate the grouping name from the path
+                            owner.name = splitOwnerPath[splitOwnerPath.length - 1];
+                        }


This logic should be on the API side, but that can be for another ticket.

JorWo · 2025-02-26T20:08:14Z

src/main/resources/static/javascript/mainApp/grouping.controller.js

+            if (listName === "owners") {
+                // If the user input has a colon, we can assume that it's a group path
+                if ($scope.manageMembers.includes(':')) {
+                    listName = "owner grouping";


There technically is no listName/groupName just for owner groupings, it would still be the owner group. Could you use a $scope.isOwnerGrouping variable instead?

@JorWo One slight issue I see with this is that the owner objects have the owner.isOwnerGrouping field and that may be confusing to some. However, if this isn't an issue I can push up the changes! (Maybe I can name it just ownerGrouping?)

Edit: Actually, it seems that a lot of the checks that use listName treats it as a string, so changing it to use a boolean in $scope.isOwnerGrouping to check would require certain functions to do a separate check just for isOwnerGrouping as well as the listName check. I can provide examples as well if needed

There are many places that in the code that use a $scope and non-scope variable to describe the same thing like $scope.listName and listName, so I assume both owner.isOwnerGrouping and $scope.isOwnerGrouping being used would not be too confusing.

Having checks for listName == "owners" && isOwnerGrouping is fine because it is still readable.

@JorWo Got it! I'll make the changes

JorWo · 2025-02-26T20:10:44Z

Are the cases handled when adding:

More than one owner grouping path?
A mix of uhIdentifiers and owner grouping paths in the input box?

src/main/resources/static/javascript/mainApp/grouping.controller.js

JorWo · 2025-02-26T20:17:43Z

src/main/resources/templates/modal/addOwnerGroupingModal.html

+    <!--
+    <p class="alert alert-warning mt-2" role="alert" ng-if="hasDeptAccount">
+        <span class="text-dark" th:text="#{screen.message.modal.add.deptAccount}"></span>
+    </p>
+
+    do I need to account for departmental accounts that are in the grouping?
+    -->


I noticed you left this comment here.

JorWo · 2025-02-26T20:18:48Z

Make sure to fix the Codacy issues, thanks.

lukepag · 2025-02-28T05:11:24Z

Are the cases handled when adding:

More than one owner grouping path?

A mix of uhIdentifiers and owner grouping paths in the input box?

I believe I had a talk with Michael about how we don't need to account for situations where users are adding more than one grouping path. @mhodgesatuh could I get your opinion on this?

Regarding the second question, it does not currently handle the mix of uhIdentifiers and owner grouping paths

mhodgesatuh · 2025-02-28T19:34:05Z

Regarding the more than one grouping path question, here's some context:
The feature will only be used by select CSOC individuals and will not be advertised to our broader audience. So the thought is that for this first iteration of the feature it is okay if we don't have rigorous user input testing. If we encounter issues, a future iteration could introduce more controls and make the feature higher profile.

JorWo · 2025-02-28T19:57:19Z

Regarding the more than one grouping path question, here's some context: The feature will only be used by select CSOC individuals and will not be advertised to our broader audience. So the thought is that for this first iteration of the feature it is okay if we don't have rigorous user input testing. If we encounter issues, a future iteration could introduce more controls and make the feature higher profile.

@mhodgesatuh Any thoughts on displaying a "feature not supported" modal if those select CSOC individuals did attempt to add more than one groupingPath owner or a mix of uhIdentifiers and groupingPaths? I want to make sure there are no disastrous consequences of not handling those cases.

mhodgesatuh · 2025-02-28T20:00:58Z

@JorWo Please update the ticket to describe at least include a minimal set of input controls to ensure that there is no unanticipated behavior. I'm fine, for example, if the initial implementation only allows for a single owner-grouping reference, in order to get minimal functionality established.

JorWo · 2025-02-28T20:09:23Z

@JorWo Please update the ticket to describe at least include a minimal set of input controls to ensure that there is no unanticipated behavior. I'm fine, for example, if the initial implementation only allows for a single owner-grouping reference, in order to get minimal functionality established.

Ok, I added a user input edge cases section to the bottom of the ticket.

add new modals for adding and removing group path owners Create new controller functions Implement new grouping owner add/remove logic Fixed add bug Implement hyperlink for grouping owners Add more comments and fix CSS spacing Create and update existing jasmine tests Finalize clean up Fix remove bug Fix pull request changes Fix more pull request changes Codacy issues Codacy

mhodgesatuh

I suggest we consistent hyphenate owner-grouper to represent the use of a grouping to indicate the indirect owners, AKA co-owners.

lukepag requested review from JorWo and mhodgesatuh February 26, 2025 05:03

mhodgesatuh reviewed Feb 26, 2025

View reviewed changes

lukepag force-pushed the dev-lukepag-1682 branch from 7d9e969 to 41ebd69 Compare February 26, 2025 19:34

JorWo reviewed Feb 26, 2025

View reviewed changes

src/main/resources/static/javascript/mainApp/grouping.controller.js Show resolved Hide resolved

JorWo reviewed Feb 26, 2025

View reviewed changes

lukepag force-pushed the dev-lukepag-1682 branch from 41ebd69 to fcf8c00 Compare February 28, 2025 05:39

lukepag force-pushed the dev-lukepag-1682 branch 2 times, most recently from f63ddbd to e244325 Compare March 1, 2025 01:45

lukepag force-pushed the dev-lukepag-1682 branch from e244325 to a0fd5f0 Compare March 1, 2025 01:56

lukepag requested review from mhodgesatuh and JorWo March 1, 2025 02:02

mhodgesatuh reviewed Mar 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add owner grouping logic to add/remove owners #1059

Add owner grouping logic to add/remove owners #1059

lukepag commented Feb 26, 2025 •

edited

Loading

mhodgesatuh left a comment

JorWo Feb 26, 2025

JorWo Feb 26, 2025 •

edited

Loading

lukepag Feb 28, 2025 •

edited

Loading

JorWo Feb 28, 2025

lukepag Mar 1, 2025

JorWo commented Feb 26, 2025 •

edited

Loading

JorWo Feb 26, 2025

JorWo commented Feb 26, 2025

lukepag commented Feb 28, 2025

mhodgesatuh commented Feb 28, 2025

JorWo commented Feb 28, 2025 •

edited

Loading

mhodgesatuh commented Feb 28, 2025

JorWo commented Feb 28, 2025

mhodgesatuh left a comment

Add owner grouping logic to add/remove owners #1059

Are you sure you want to change the base?

Add owner grouping logic to add/remove owners #1059

Conversation

lukepag commented Feb 26, 2025 • edited Loading

Ticket Link

List of squashed commits

Test Checklist

mhodgesatuh left a comment

Choose a reason for hiding this comment

JorWo Feb 26, 2025

Choose a reason for hiding this comment

JorWo Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

lukepag Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

JorWo Feb 28, 2025

Choose a reason for hiding this comment

lukepag Mar 1, 2025

Choose a reason for hiding this comment

JorWo commented Feb 26, 2025 • edited Loading

JorWo Feb 26, 2025

Choose a reason for hiding this comment

JorWo commented Feb 26, 2025

lukepag commented Feb 28, 2025

mhodgesatuh commented Feb 28, 2025

JorWo commented Feb 28, 2025 • edited Loading

mhodgesatuh commented Feb 28, 2025

JorWo commented Feb 28, 2025

mhodgesatuh left a comment

Choose a reason for hiding this comment

lukepag commented Feb 26, 2025 •

edited

Loading

JorWo Feb 26, 2025 •

edited

Loading

lukepag Feb 28, 2025 •

edited

Loading

JorWo commented Feb 26, 2025 •

edited

Loading

JorWo commented Feb 28, 2025 •

edited

Loading