Comment 2 for bug 2043218

Hi Terran! Thanks for the patch and the test plan.

I don't think the permission is unused; I think it's intended as the generic update_perm for org unit settings. In my environment, we only really want library admins to be able to update most org settings, so we assign UPDATE_ORG_SETTING to our library admin permission groups and then use that as the update_perm for those settings. For settings where we need more granular permission control, we use a different update_perm, but it's handy to have a generic option. So in my opinion, we should keep the perm.

Looking at the patch itself, I see one small problem and one bigger one.

The small problem is that we generally don't modify past upgrade scripts, so I think the changes to Open-ILS/src/sql/Pg/upgrade/0026.data.reserve_perm_list_id_range.sql and Open-ILS/src/sql/Pg/version-upgrade/1.6.1-2.0-upgrade-db.sql should be omitted.

The bigger problem is these three lines in the upgrade script:

DELETE FROM config.org_unit_setting_type WHERE view_perm = uosid OR update_perm = uosid;
DELETE FROM config.remoteauth_profile WHERE perm = uosid;
DELETE FROM config.z3950_source WHERE use_perm = uosid;

The intention here is to stop using the perm anywhere it's in use, but what these statements are actually doing is deleting any rows from those tables that happen to use the perm. For example, if I have an org setting that uses UPDATE_ORG_SETTING as an update_perm, that first line won't remove the update_perm, it will delete the setting altogether! Rather than deleting rows from these tables, you would need to set the permission to some different value. You could set it to NULL, but that would probably leave things in an undesirable state (if I was using UPDATE_ORG_SETTING as an update_perm, setting update_perm to null means now anybody can update those settings, which isn't what I want).

The remaining DELETE statements, from permission.grp_perm_map and so on, are OK because you're just deleting mappings for the perm; there are no unwanted side effects in that case.

Hope that's helpful! Let me know if I can clarify anything.