#3217: fix/rethink valueChange subscription for onebox values

[kshepherd]

References

• Fixes Faulty submission form: arrange more than 3 lines #3217

Description

This is a PR that demonstrates where the problem is occurring with controlled onebox value re-ordering -- there is a subscription to valueChanges from the model which updates currentValue of the onebox, but it uses this.model.id as a persistent index in the group array, which is not hte case -- after a reorder, this id will not match the real index.

My fix here might not be suitable, and I ask some experienced submission angular devs for their input - was the intention here to listen for changes to the workspace item while the submission form is open, beteween saves?

If we need to leave this subscription in (and I accept there might be good reasons), we need to find a way to track a current index for the onebox, or regenerating all the ids after a reorder.

In the meantime, those users suffering from #3217 or #2004 issues, related to reordering of onebox values in submission (where the fields are configured for vocab/auth), might like to test out this small fix and see if it fixes the problem.

Instructions for Reviewers

Without this PR, observe what happens when onebox values are reordered - I suggest author and subject, using default configuration - as per #3217

With this PR applied, try the same thing, and also ensure that submission saves / reloads / deposits perform as expected with no new side-effects.

For the advanced tester, make a change to one of the values of the workspace item in question using just the REST API, and see what happens to the active submission form with/without the PR applied

Notes for Reviewers

I am keen to hear from Angular submission devs to make sure I understand the intention of this subscription, and if it is necessary, discuss the best way to track the index so we can put the subscription back safely.

List of changes in this PR:

• Remove subscription to model valueChanges from onebox component

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes ESLint validation using yarn lint
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
• If my PR fixes an issue ticket, I've linked them together.

[tdonohue]

@atarix83 or @artlowel : This small fix would be good for one of you to glance at, since @kshepherd is removing subscriptions (from the dynamic-onebox.component.ts) which he feels are causing issues. So, one of you may want to look at this to be sure you agree with the approach.

[AndreaBarbasso]

I was looking at the same issue but in another component (dynamic-scrollable-dropdown) and I came to the same conclusion as @kshepherd. I can't really find a reason for these subscriptions (the dynamic-scrollable-dropdown has a similar subscription to the one found in dynamic-onebox) and I see why it's causing issues.

If this PR turns out to be ok and accepted, I would follow the same practice on the dynamic-scrollable-dropdown component and remove its subscription as well!