Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Understanding getInitValueFromModel with values that do not have authority keys #3150

Open
kshepherd opened this issue Jun 27, 2024 · 3 comments
Labels
component: authority control Related to internal authority control system question

Comments

@kshepherd
Copy link
Member

kshepherd commented Jun 27, 2024

(This is not a bug report, more a request for clarification to help me understand why we are searching vocabulary for best match in a particular situation. I will update the inline doc with some more explanation, and if it turns out we think we should fix or improve something, then we can update this issue.)

I have been working on some vocabulary features lately, and I ran into an odd issue where upon form load of an existing submission (or, e.g. refresh of the page), any plain text values that were saved in onebox fields configured for non-closed vocabularies would attempt to search that vocabulary and then overwrote the existing value with a new FormFieldMetadataValueObject as though I had intentionally done a search and selected a result.
But there was no user action, it was just taking the non-authority-controlled value in a free text box and overwriting it with the result of getBestMatch in the backend vocabulary service.

The code is in DsDynamicVocabularyComponent.getInitValueFromModel.

This seems a bit wrong or unexpected to me - in my case, I want to allow free text entry as well as lookups of controlled values in my external data source, but I want to preserve the free text values as plain metadata and not search the vocabulary and replace the stored values without a user action.

Can someone help explain the intention behind the initEntry$ = this.vocabularyService.getVocabularyEntryByValue(this.model.value.value, this.model.vocabularyOptions); call? Was it intended for really simple controlled vocabulary XML where one could expect a single exact match to reliably be returned for e.g. a subject term?

Is this something we should change, or do I just need to understand it better?

I have currently "fixed" the behaviour in my custom code by moving the 'hasAuthority' check to the outer conditional, so if there is no authority key at all, the init value is just the text from the form, with no search performed and no authority/confidence set in the object. Are there any side-effects I am missing?

Thanks!

  /**
   * Retrieves the init form value from model
   * @param preserveConfidence if the original model confidence value should be used after retrieving the vocabulary's entry
   */
  getInitValueFromModel(preserveConfidence = false): Observable<FormFieldMetadataValueObject> {
    let initValue$: Observable<FormFieldMetadataValueObject>;
    if (isNotEmpty(this.model.value) && (this.model.value instanceof FormFieldMetadataValueObject) && !this.model.value.hasAuthorityToGenerate()) {
      let initEntry$: Observable<VocabularyEntry>;
      if (this.model.value.hasAuthority()) {
        initEntry$ = this.vocabularyService.getVocabularyEntryByID(this.model.value.authority, this.model.vocabularyOptions);
      } else {
        initEntry$ = this.vocabularyService.getVocabularyEntryByValue(this.model.value.value, this.model.vocabularyOptions);
      }
      initValue$ = initEntry$.pipe(map((initEntry: VocabularyEntry) => {
        if (isNotEmpty(initEntry)) {
          // Integrate FormFieldMetadataValueObject with retrieved information
          const formField = new FormFieldMetadataValueObject(
            initEntry.value,
            null,
            initEntry.authority,
            initEntry.display,
            (this.model.value as any).place,
            null,
            initEntry.otherInformation || null,
          );
          // Preserve the original confidence
          if (preserveConfidence) {
            formField.confidence = (this.model.value as any).confidence;
          }
          return formField;
        } else {
          return this.model.value as any;
        }
      }));
    } else if (isNotEmpty(this.model.value) && (this.model.value instanceof VocabularyEntry)) {
      initValue$ = observableOf(
        new FormFieldMetadataValueObject(
          this.model.value.value,
          null,
          this.model.value.authority,
          this.model.value.display,
          0,
          null,
          this.model.value.otherInformation || null,
        ),
      );
    } else {
      initValue$ = observableOf(new FormFieldMetadataValueObject(this.model.value));
    }
    return initValue$;
  }
@kshepherd kshepherd added bug needs triage New issue needs triage and/or scheduling question component: authority control Related to internal authority control system and removed bug needs triage New issue needs triage and/or scheduling labels Jun 27, 2024
@kshepherd
Copy link
Member Author

Hi @atarix83 just pinging you on this question since I think you were involved with the original implementation. Cheers!

@tdonohue
Copy link
Member

tdonohue commented Jun 27, 2024

@kshepherd : Based on "blame" in Github, it looks like that line of code was added by @atarix83 :

https://github.com/DSpace/dspace-angular/blob/main/src/app/shared/form/builder/ds-dynamic-form-ui/models/dynamic-vocabulary.component.ts#L69

I believe it was added very early in 7.0 work, in this PR #751 in the very first version of this code.

I'm not certain myself of the original purpose, but it's possible you've found a bug in the behavior. (It's also possible that it had a purpose initially but now no longer serves the same purpose after later code refactoring/changes)

Based on your description though, it sounds like a bug to me. If I was a submitter, wouldn't want my plain text value to be replaced automatically with a controlled vocabulary value.

@kshepherd
Copy link
Member Author

My best guess is that it was for early cases where we had no external data / authority plugged in, just plain XML controlled values and it was a 'helper' thing to get a value like 'Mathematics' auto-associated with the subject vocab entry that had that exact name, or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: authority control Related to internal authority control system question
Projects
Status: 👀 Needs Discussion / Analysis
Development

No branches or pull requests

2 participants