actor.org.id has type "org_unit" in the IDL, not "id"

[Galen Charlton]

This was inspired by bug 2051879.

The actor.org_unit.id column is given the type "org_unit" in the IDL rather than "id". As a consequence, the Angular FmRecordEditorComponent renders it as a selector, when as a (non-editable) primary key it should be rendered as a non-editable display of the ID number.

It's been this way since at least 2009.

While this seems like a straightforward thing to fix, if anybody knows of a reason why this unique arrangement is needed, please advise.

Evergreen 3.11+

[Galen Charlton (gmc)]

A list of things that might be affected by changing the type:

- Angular OU selector
- Angular FM record editor
- AngularJS OU selector
- AngularJS FM record editor
- Dojo AutoFieldWidget in any legacy interfaces still in use
- Report templates

[Terran McCanna (tmccanna)]

I'm most concerned about how it would impact report templates if the change would require rebuilding existing templates. (We have thousands.)

[Mike Rylander (mrylander)]

The type of "org_unit" on the pkey of aou in the IDL is, indeed, intentional. The original purpose, inherited by many other parts of the code to the same basic effect, is to indicate that an org selector of some sort should be displayed for that column in the report definition editor.

That's still necessary for the reporter, and I think showing the org selector is the correct behavior for any UIs /except/ for the FM editor when it's loading an org unit. There probably aren't many of those, other than grids, but report definitions do definitely need that marker.

In the case of the FM editor, I actually think it's using the wrong test. Instead of looking at the datatype of id, I think it should be looking at the class's primary key field from the "persist"
XML namespace (//idl:fields/@oils_persist:primary) rather than the "id" datatype. Perhaps like this:

diff --git a/Open-ILS/src/eg2/src/app/share/fm-editor/fm-editor.component.ts b/Open-ILS/src/eg2/src/app/share/fm-editor/fm-editor.component.ts
index 07fdff15cc..a8347b9245 100644
--- a/Open-ILS/src/eg2/src/app/share/fm-editor/fm-editor.component.ts
+++ b/Open-ILS/src/eg2/src/app/share/fm-editor/fm-editor.component.ts
@@ -664,6 +664,10 @@ export class FmRecordEditorComponent
             return 'timestamp-timepicker';
         }

+ if (this.idlDef.pkey === field.name && !this.pkeyIsEditable) {
+ return 'readonly';
+ }
+
         // Some widgets handle readOnly for us.
         if ( field.datatype === 'timestamp'
             || field.datatype === 'org_unit'

If we don't want to change the general "is this an editable pkey" logic in FMEC entirely, which I would understand, then a special-case for aou similar to what exists for the user-link test is probably the less disruptive change overall. Something like:

diff --git a/Open-ILS/src/eg2/src/app/share/fm-editor/fm-editor.component.ts b/Open-ILS/src/eg2/src/app/share/fm-editor/fm-editor.component.ts
index 07fdff15cc..6dfe8cdf7c 100644
--- a/Open-ILS/src/eg2/src/app/share/fm-editor/fm-editor.component.ts
+++ b/Open-ILS/src/eg2/src/app/share/fm-editor/fm-editor.component.ts
@@ -664,6 +664,10 @@ export class FmRecordEditorComponent
             return 'timestamp-timepicker';
         }

+ if (this.idlClass === 'aou' && field.name === 'id') {
+ return 'readonly';
+ }
+
         // Some widgets handle readOnly for us.
         if ( field.datatype === 'timestamp'
             || field.datatype === 'org_unit'

I lean toward the first option. It seems safe to me and is more general since it doesn't depend on anything but the structure of the IDL and the existing "surrogate value pkeys are not editable" rule embodied by pkeyIsEditable calculation, but does anyone else have thoughts on those?

[Galen Charlton (gmc)]

Thanks for the background, Mike. At first blush, the first option works for me (along with adding a comment to fm_IDL.xml to explain what's going on).

[Mike Rylander (mrylander)]

Thanks, Galen. In that case, it's dangerous to go alone! Take this:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp-2051944-aou_id-widget-in-FMREC

[Galen Charlton (gmc)]

Tested and signed off. Branch is user/gmcharlt/lp2051944_signoff

[Terran McCanna (tmccanna)]

Thanks Mike and Galen! Also tested, and committed back to rel_3_11.