fix(submission): keep dc.type dropdown display after change#157
Open
kosarko wants to merge 3 commits into
Open
fix(submission): keep dc.type dropdown display after change#157kosarko wants to merge 3 commits into
kosarko wants to merge 3 commits into
Conversation
(ufal/clarin-dspace#1377 -> #156) The "type of resource" scrollable dropdown blanked out after changing its value when the backend was slow: the reloaded model value came back with an empty display but populated value/authority, so currentValue rendered ''. Fall back to the underlying value when display is empty, matching the existing inputFormatter convention. Add a fakeAsync regression test that drives an empty-display value through group.valueChanges (the real reload entry point) and asserts the rendered input keeps its text, reproducing the original blank-after-latency bug.
There was a problem hiding this comment.
Pull request overview
Fixes a submission UI regression where the dc.type “type of resource” scrollable dropdown could blank out after a value change when the reloaded value had an empty display but a populated value/authority. The component now falls back to the underlying value when display is empty, consistent with the existing inputFormatter behavior.
Changes:
- Update
setCurrentValueto usedisplay || value(both on init and on subsequent value changes). - Add unit/regression tests (including a
fakeAsynccase) to ensure the rendered<input>keeps its text when a value with emptydisplayarrives viavalueChanges.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts | Ensures currentValue displayed in the dropdown input falls back to value when display is empty (init + updates). |
| src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.spec.ts | Adds regression coverage for empty-display values, including the valueChanges-driven reload scenario. |
| }); | ||
|
|
||
| it('should fall back to the underlying value when display is empty on init', () => { | ||
| spyOn(vocabularyServiceStub, 'getVocabularyEntryByID').and.returnValue(observableOf(new VocabularyEntry())); |
Address Copilot PR review: the init-path test previously relied on isNotEmpty(new VocabularyEntry()) being false so getInitValueFromModel would fall back to the raw model.value. That couples the test to VocabularyEntry having no initialized fields. Stub getInitValueFromModel directly to emit an empty-display value (built bypassing the FFMVO constructor, which would otherwise fill display via `display || value`), so the test deterministically exercises the setCurrentValue fallback it is meant to guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The DSDynamicTypeBindRelationService spec called spyOn() on formBuilderService.getTypeBindModel / getTypeBindModelUpdates, but the mock (getMockFormBuilderService) already creates those as jasmine spies via createSpyObj. Because src/test.ts runs with teardown.destroyAfterEach:false and karma randomizes spec order, certain orderings hit the second spy installation and threw "getTypeBindModel has already been spied upon", failing 6 tests non-deterministically (green on clarin-v7, red here only due to a different random ordering). Reconfigure the existing spies with .and.* instead of re-spying, which is order-independent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves #156
(originally as ufal/clarin-dspace#1377)
Add a fakeAsync regression test that drives an empty-display value through group.valueChanges (the real reload entry point) and asserts the rendered input keeps its text, reproducing the original blank-after-latency bug.
Problem description
The "type of resource" scrollable dropdown blanked out after changing its value when the backend was slow: the reloaded model value came back with an empty display but populated value/authority, so currentValue rendered ''. Fall back to the underlying value when display is empty, matching the existing inputFormatter convention.
Copilot review