OBPIH-4445 Allow deactivation of organization records #3407

kchelstowski · 2022-08-10T15:19:39Z

As discussed with Justin, I did not touch location.active, but created a separate property called status which is determined by the location.active and its associated organization.active:

LocationStatus getStatus() {
        if (organization) {
            if (active && organization.active) {
                return LocationStatus.ENABLED
            }
            return LocationStatus.DISABLED
        }
        return active ? LocationStatus.ENABLED : LocationStatus.DISABLED
    }

for that, I created an enum called LocationStatus with two statuses: ENABLED and DISABLED. I just thought it would be better to do it this way instead of having as status boolean values (it would look weird having status: true, wouldn't it?)
Doing so, I also added one column to location view page called Status so we can see if a location is enabled or disabled (by disabled it could mean that it is active itself, but the associated organization is disabled). For the locations that are not associated with any organization, I assumed they should be ENABLED (if they are obviously active themselves).

I also modified some location getters, so you do not see disabled locations on location chooser (I provided changes and tested for all places: gsp, react, chooseLocation page).
Moreover I modified organization getters, so you don't see inactive organizations when editing a location on Organization's dropdown, when creating a new location.
I also added an additional validation in case you are creating a location and you choose an active organization, but the organization is being disabled in the mean time.
I'm looking forward to your reviews @jmiranda @awalkowiak

jmiranda · 2022-08-10T21:29:03Z

grails-app/conf/SecurityFilters.groovy

@@ -117,6 +118,12 @@ class SecurityFilters {
                    redirect(controller: 'auth', action: 'login')
                    return false
                }
+                // Check if a user is logged to disabled location (location.active or organization.active is false) - if so, redirect to location chooser
+                Location currentLocation = Location.get(session?.warehouse?.id)


Let's get the current location from the thread local (which is set on line 49 of SecurityFilters)

Location currentLocation = AuthService.currentLocation.get()

If there are cases where this returns null (i.e. this line of code is invoked before line 49) then we could so something like this.

Location currentLocation = AuthService.currentLocation.get()?:Location.get(session?.warehouse?.id)

I want to avoid doing multiple queries for the same data in the same request.

jmiranda · 2022-08-10T21:33:55Z

grails-app/controllers/org/pih/warehouse/api/LocationApiController.groovy

@@ -146,6 +147,11 @@ class LocationApiController extends BaseDomainApiController {
            location.supportedActivities.clear()
        }

+        // If the organization chosen for the created location is inactive, throw an exception
+        if (!location.organization.active) {


Organization is nullable so you'll want to use null protection (?.) here.

Can you explain why we need this validation here? Won't we get this for free from the validation in the security filters? I assume it's because you need to throw an exception here because otherwise the API request is going through the security filter, getting the session.warehouse attribute set to null and then going on its way. Whereas we need the API request to fail. However, I believe we need requests that are associated with Location that is disabled should always fail. Whether they are creating, editing, deleting the location.

And I assume we don't necessarily want every API endpoint to have to add this check. A more generic way of handling this would be to have the security filter check whether the request is a web request or API request and then handle the response appropriately. This way we keep all of the authorization / validation in the same place.

// ignore the name of the method, this was primarily used for ajax request but i think it works for API requests as well. if (RequestUtil.isAjax(request)) { throw new IllegalArgumentException("The organization ${location.organization.name} is inactive, you can't assign it to the location") } else { flash.message = "Your location is disabled (either the associated organization or your location itself became inactive)" }

The validation in the security filters only checks if your current location's organization is inactive - the validation here checks if you are creating a location and trying to assign an inactive organization to it (you are not logged in to the location you are creating, so the validation in SecurityFilters has nothing to this one in my opinion).
I also don't think doing it in generic way would be the best idea as there might be cases, where we would want to use an inactive organization.

jmiranda · 2022-08-10T21:47:32Z

grails-app/domain/org/pih/warehouse/core/Location.groovy

+            }
+            return LocationStatus.DISABLED
+        }
+        return active ? LocationStatus.ENABLED : LocationStatus.DISABLED


I think this might be more readable.

if (organization) { return active && organization.active ? LocationStatus.ENABLED : LocationStatus.DISABLED } else { return active ? LocationStatus.ENABLED : LocationStatus.DISABLED }

Will correct it to this:

if (organization) { return (active && organization.active) ? LocationStatus.ENABLED : LocationStatus.DISABLED } return active ? LocationStatus.ENABLED : LocationStatus.DISABLED

You don't need to use else here - if the return from inside if is executed, it doesn't go any further, so it doesn't execute return from the bottom

jmiranda · 2022-08-10T21:51:37Z

grails-app/services/org/pih/warehouse/core/LocationService.groovy

@@ -114,6 +114,10 @@ class LocationService {
        def locations = new HashSet()
        locations += getLocations(fields, params)

+        if (params.locationChooser) {


I don't love this. A more generic way would be better. Like adding a backwards compatible boolean flag to the method. We don't need to change any of the existing calls to this method, but we can override the one we need to.

def getLocations(String[] fields, Map params, Boolean isSuperuser, String direction, Location currentLocation, User user, Boolean excludeDisabled = Boolean.FALSE) { def locations = new HashSet() locations += getLocations(fields, params) if (excludeDisabled) { locations = locations.findAll { Location location -> location.status == LocationStatus.ENABLED } }

@awalkowiak do you have any thoughts?

Yeah, I think I like your version more. In the current version, we are making decisions about what we want to pull based on the params.locationChooser in the controller. @kchelstowski let's go with this version for now.

Btw @jmiranda there is already a tech debt ticket created for refactoring the location API (OBPIH-4594), perhaps we could squeeze it in the 0.8.20.

jmiranda · 2022-08-10T21:53:09Z

grails-app/services/org/pih/warehouse/core/LocationService.groovy

-
-                ]
-            }
+            locations = locations.findAll{ Location location -> location.status == LocationStatus.ENABLED }


Same here, we should probably add a new excludeDisabled boolean argument (with default value = false) to the method.

jmiranda · 2022-08-10T21:54:01Z

grails-app/services/org/pih/warehouse/core/OrganizationService.groovy

@@ -17,7 +17,7 @@ class OrganizationService {
    boolean transactional = true


-    List selectOrganizations(roleTypes) {
+    List selectOrganizations(roleTypes, active = false) {


It was a bad idea to leave these untyped, so go ahead and add Boolean and a type for the roleType argument.

jmiranda · 2022-08-10T21:58:37Z

@kchelstowski By the way, great job with this one. I think you really had a good grasp of all of the issues.

awalkowiak · 2022-08-11T10:12:47Z

grails-app/services/org/pih/warehouse/core/LocationService.groovy

@@ -114,6 +114,10 @@ class LocationService {
        def locations = new HashSet()
        locations += getLocations(fields, params)

+        if (params.locationChooser) {


Yeah, I think I like your version more. In the current version, we are making decisions about what we want to pull based on the params.locationChooser in the controller. @kchelstowski let's go with this version for now.

Btw @jmiranda there is already a tech debt ticket created for refactoring the location API (OBPIH-4594), perhaps we could squeeze it in the 0.8.20.

awalkowiak · 2022-08-11T10:13:21Z

grails-app/services/org/pih/warehouse/core/LocationService.groovy

-
-                ]
-            }
+            locations = locations.findAll{ Location location -> location.status == LocationStatus.ENABLED }


awalkowiak · 2022-08-11T10:14:14Z

grails-app/views/location/list.gsp

+                                            <td class="middle">
+                                                <g:if test="${locationInstance?.status == LocationStatus.ENABLED }">
+                                                    <span class="active">
+                                                        ENABLED


Could you localize it?

awalkowiak · 2022-08-11T10:14:19Z

grails-app/views/location/list.gsp

+                                                </g:if>
+                                                <g:else>
+                                                    <span class="inactive">
+                                                        DISABLED


Could you localize it?

awalkowiak · 2022-08-11T10:23:29Z

grails-app/views/location/list.gsp

@@ -107,6 +108,18 @@
                                            </td>
                                            <td class="left middle"><format:metadata obj="${locationInstance?.locationType}"/></td>
                                            <td class="left middle">${locationInstance?.locationGroup?:warehouse.message(code:'default.none.label')}</td>
+                                            <td class="middle">
+                                                <g:if test="${locationInstance?.status == LocationStatus.ENABLED }">
+                                                    <span class="active">


This could be as simple as:

<span class="${locationInstance?.status == LocationStatus.ENABLED ? 'active' : 'inactive' }"> <format:metadata obj="${locationInstance?.status}"/> </span>

kchelstowski · 2022-08-11T12:55:47Z

I pushed fixes. I also in the mean time found some minor "bugs" - for example when a location had inactive organization and you went to location edit, the current (inactive) organization was not fetched and was not assigned as initial value to dropdown, so I made some changes to fix that, because the user would think that the location he/she is editing doesn't have any organization assigned.
I also changed validation a bit, so if a user is editing the location which has inactive organization, the user is able to proceed "Update" on edit page (without this change, if the user wanted to press "Update" without making any changes, the validation would tell to the user that this organization is inactive and he is not able to assign it, but de facto the user didn't change anything). I know it sounds complicated, but I didn't know how to explain it better.
I'm also terrified about those location getters. I'm not happy about this excludeDisabled, because there are many places where we'd want to have only enabled locations being fetched, but there are too many methods: getLoginLocations, getLocations, getLoginLocationsMap etc, so I wouldn't be surprised if I hadn't provided this additional flag excludeDisabled everywhere where it's needed. Having params.locationChooser inside the method seemed better idea for me, because there is 99.99% chance, that where we want to get locationChooser locations, this param is proceeded, but remembering about adding excludeDisabled flag to each trigger of the method could be tricky.
As @awalkowiak said, he had already prepared a ticket to refactor this stuff and I hope it will happen soon.

cc @awalkowiak @jmiranda

awalkowiak · 2022-08-11T14:46:49Z

grails-app/controllers/org/pih/warehouse/api/LocationApiController.groovy

@@ -146,6 +147,11 @@ class LocationApiController extends BaseDomainApiController {
            location.supportedActivities.clear()
        }

+        // If the organization chosen for the created location is inactive, throw an exception
+        if (!location.organization?.active) {


AFAIR non-depots and non-suppliers locations do not need to have an organization, so this should only happen if a location has an organization.

that's nice catch, thanks, actually funny thing is that in other place

if (pickedOrganization && (pickedOrganization?.id != currentOrganization?.id) && !pickedOrganization?.active)

I remembered about it and did that check

awalkowiak · 2022-08-11T16:12:41Z

grails-app/controllers/org/pih/warehouse/core/LocationController.groovy

@@ -83,6 +83,11 @@ class LocationController {
                }
            }

+            Organization currentOrganization = locationInstance.organization
+            Organization pickedOrganization = Organization.get(params.organization?.id)
+            if (pickedOrganization && (pickedOrganization?.id != currentOrganization?.id) && !pickedOrganization?.active) {


What if someone updates a location that has an organization that was deactivated before the location update? Should we also throw this exception to the user? In this version, this won't happen. Perhaps it is enough to do:

Organization organization = Organization.get(params.organization?.id) if (organization && !organization.active) { throw exception }

Also - what if at this point there is no params.organization?.id and in line 96, the findOrCreateSupplierOrganization will find an inactive organization?

What if someone updates a location that has an organization that was deactivated before the location update?

@awalkowiak I actually didn't want to it to cause an error, because a user might be confused when he/she goes to edit page, but decides to leave it as it is and just presses Update (but doesn't change the current organization. In my opinion it would be natural instinct to press Update as long as we don't have Go back or something like this button) and suddenly gets an error.
I think there would be no consequence if we allow a location to have an inactive organization (not while editing, but just by making already associated organization inactive), but at the same time we would punish/force the user to change the organization if he/she by mistake clicks Edit Location.
I just wanted to make sure, that if user doesn't change the organization while updating the location (which in the mean time could be set as inactive) and just keeps the 'old` organization, it would not cause the error.

That's my thought, I don't say it's the best idea or approach, it was just my point of view and that's why the code looks like this - it was done intentionally.
Let me know if I should leave it as it is or change to your approach and version.

Make sure Katyrzyna reads these comments so she knows about some of the edge cases we've been discussing.

jmiranda

Sorry, I need more time to review. Will continue tomorrow.

jmiranda

Small changes requested. I'll merge tomorrow after these are done.

jmiranda · 2022-08-16T06:02:47Z

grails-app/controllers/org/pih/warehouse/core/LocationController.groovy

@@ -83,6 +83,11 @@ class LocationController {
                }
            }

+            Organization currentOrganization = locationInstance.organization
+            Organization pickedOrganization = Organization.get(params.organization?.id)


Sorry for the nitpick but let's called this selectedOrganization. The term "picked" has a different meaning in a WMS so I don't want to confuse anyone.

jmiranda · 2022-08-16T06:05:36Z

grails-app/controllers/org/pih/warehouse/core/LocationController.groovy

@@ -83,6 +83,11 @@ class LocationController {
                }
            }

+            Organization currentOrganization = locationInstance.organization
+            Organization pickedOrganization = Organization.get(params.organization?.id)
+            if (pickedOrganization && (pickedOrganization?.id != currentOrganization?.id) && !pickedOrganization?.active) {


Make sure Katyrzyna reads these comments so she knows about some of the edge cases we've been discussing.

jmiranda · 2022-08-16T06:06:06Z

grails-app/domain/org/pih/warehouse/core/Organization.groovy

@@ -21,6 +21,7 @@ class Organization extends Party {
    String name
    String description
    Location defaultLocation
+    Boolean active = true   // by default = true


You can remove the comment here.

jmiranda · 2022-08-16T06:09:26Z

I'm also terrified about those location getters. I'm not happy about this excludeDisabled, because there are many places where we'd want to have only enabled locations being fetched, but there are too many methods: getLoginLocations, getLocations, getLoginLocationsMap etc, so I wouldn't be surprised if I hadn't provided this additional flag excludeDisabled everywhere where it's needed. Having params.locationChooser inside the method seemed better idea for me, because there is 99.99% chance, that where we want to get locationChooser locations, this param is proceeded, but remembering about adding excludeDisabled flag to each trigger of the method could be tricky.

The purpose of the excludeDisabled argument is to provide a way override the default behavior which is to include disabled. If we think the default behavior should be to exclude disabled by default then we don't need this flag. However, that means we'll never be able to include the disabled locations.

…ns with inactive org

OBPIH-4445 Allow deactivation of organization records

e75c93a

kchelstowski requested review from jmiranda and awalkowiak August 10, 2022 15:19

jmiranda requested changes Aug 10, 2022

View reviewed changes

awalkowiak requested changes Aug 11, 2022

View reviewed changes

OBPIH-4445 Fixes after review and minor additional changes

27e88c9

kchelstowski requested review from awalkowiak and jmiranda August 11, 2022 12:55

OBPIH-4445 Fix tests

fc32ef1

awalkowiak requested changes Aug 11, 2022

View reviewed changes

jmiranda reviewed Aug 12, 2022

View reviewed changes

OBPIH-4445 Add check if created location has an organization

69366c4

jmiranda requested changes Aug 16, 2022

View reviewed changes

OBPIH-4445 Fixes after review and added validation for import locatio…

9f6b0c6

…ns with inactive org

kchelstowski requested review from jmiranda and awalkowiak August 16, 2022 08:21

awalkowiak approved these changes Aug 16, 2022

View reviewed changes

awalkowiak merged commit 1e7de03 into develop Aug 16, 2022

awalkowiak deleted the OBPIH-4445 branch August 16, 2022 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-4445 Allow deactivation of organization records #3407

OBPIH-4445 Allow deactivation of organization records #3407

kchelstowski commented Aug 10, 2022 •

edited

jmiranda Aug 10, 2022

jmiranda Aug 10, 2022

jmiranda Aug 10, 2022

kchelstowski Aug 11, 2022 •

edited

jmiranda Aug 10, 2022

kchelstowski Aug 11, 2022 •

edited

jmiranda Aug 10, 2022

awalkowiak Aug 11, 2022

jmiranda Aug 10, 2022

awalkowiak Aug 11, 2022

jmiranda Aug 10, 2022

jmiranda commented Aug 10, 2022

awalkowiak Aug 11, 2022

awalkowiak Aug 11, 2022

awalkowiak Aug 11, 2022

awalkowiak Aug 11, 2022

awalkowiak Aug 11, 2022

kchelstowski commented Aug 11, 2022

awalkowiak Aug 11, 2022

kchelstowski Aug 11, 2022

awalkowiak Aug 11, 2022

kchelstowski Aug 11, 2022 •

edited

jmiranda Aug 16, 2022

jmiranda left a comment

jmiranda left a comment

jmiranda Aug 16, 2022

jmiranda Aug 16, 2022

jmiranda Aug 16, 2022

jmiranda commented Aug 16, 2022

OBPIH-4445 Allow deactivation of organization records #3407

OBPIH-4445 Allow deactivation of organization records #3407

Conversation

kchelstowski commented Aug 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski Aug 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski Aug 11, 2022 • edited

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

jmiranda commented Aug 10, 2022

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

kchelstowski commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski Aug 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmiranda left a comment

Choose a reason for hiding this comment

jmiranda 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

jmiranda commented Aug 16, 2022

kchelstowski commented Aug 10, 2022 •

edited

kchelstowski Aug 11, 2022 •

edited

kchelstowski Aug 11, 2022 •

edited

kchelstowski Aug 11, 2022 •

edited