DSpace / dspace-angular

DSpace User Interface built on Angular.io
https://wiki.lyrasis.org/display/DSDOC8x/
BSD 3-Clause "New" or "Revised" License
122 stars 393 forks source link

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

Open kshepherd opened 1 week ago

kshepherd commented 1 week ago

(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 commented 1 week ago

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

tdonohue commented 1 week ago

@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 https://github.com/DSpace/dspace-angular/pull/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 commented 1 week ago

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.