Before describing the approach adopted in this PR, it is useful to summarise the problems that it directly or indirectly tries to address.
Having investigated them for a long time, it has been possible to trace the root causes of these problems.
Problems caused by the template/zero row of repeatable fields
(1) Errors cannot be displayed correctly
Even if the field is correct from an application point of view, the template row is a real form field and follows the validation rules attached to the repeatable field. For this reason we can see a section-level warning. #931
Problems caused by the repeatable field without form redrawing
(2) By clicking on ADD the repeatable field is artificially patched.
This approach forces the native flow from the used angular library and doesn't work with many of the models implemented in DSPACE, which would need custom patching logic. #948
Problems caused by the repeatable field with redraw
(3) The content in the template/zero row is deleted. #864 #884
Problems caused by lookup functionality
(4) The redraw of the form is mandatory.
The selected value is not added to the form, but a save submission is evoked. Once the data returns from the server, the form is redrawn.
Problems caused by form redraw
(5) The history of the form is deleted, bypassing the native angular logic for displaying errors, which is typically very simple to implement by exploiting the touched and pristine fields of the controls.
(6) In addition, fields edited but not saved (like during an autosave) are emptied.
At this moment all the above problems except (5), (6) and (3) (which is a side effect) are mitigated by the redraw which is extremely frequent. The more you reduce the frequency of redraw, the more these problems arise.
But it is clear that redraw in turn causes many side effects and ideally should be avoided as much as possible.
Current workaround to all this mess
Notifications to the user have been reduced to only manual saves (by clicking on the submission's footer buttons)
The Save button is only displayed as enable if there are unsaved changes.
The hasMetadataEnrichment check responsible to avoid form redraw has been reintroduced by inserting the metadata submission in the store to limit the redraw. This with the aim of reducing (3).
Unfortunately, as said, this has highlighted a number of weaknesses linked to (2). Solving these kind of problems is out of the scope of this issue, and it would be a very expensive and not very scalable job because it involves writing code for each model, and not very maintainable because in addition to having a lot of code to mantain it forces the native behavior of the library that should be able to be upgraded easily.
We did not consider convenient to proceed with solving (3) by saving the values inserted in the first line, unless before a deep analysis and discussion with the community. The problem is the same, as well as making the store heavier it requires the implementation of specific patches for each model, an approach that is difficult to maintain and not scalable.
Since frequent or infrequent redraw is a scenario to deal with in any case, the information of touched fields has been added for the correct display of errors. But this is all made in vain by (1). Solving this problem without intervening directly on the repeatable field by rethinking it, becomes extremely difficult, unless you rewrite from scratch the error support already provided by angular (which is one of the most valuable aspects of the framework).
Moreover:
We tried to save the focused state of the section in order to postpone the form redraw which causes the emptying of populated fields and the loss of the focus field (6).
The result is that sometimes the redraw is not evoked at all as the user goes from one field to another without ever leaving the form. So we decided to revert (but still in commit history).
It doesn't solve in any way (1) whose fix, as already stated, is expensive to implement and unmaintainable.
Ideal solution: remove the template row from the repeatable field control
Removing the template row from the repeatable fields immediately fix (1), (2), (3).
The added submission's metadata into the store reduces the redraw so that the user experience is fluid most of the time. The redraw remains in case of interaction with lookups.
Moreover
Implementing the lookup so that it directly patches the form could be more than welcome.
Leave the redraw as the only strategy in case the server actually enriches the form with additional data.
In that case a notification will be displayed to the user and it would probably be acceptable to lose the information about the touched fields and have a solution more clean and mantainable.
Checklist
[ ] 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.
[x] My PR passes TSLint validation using yarn run lint
[ ] My PR doesn't introduce circular dependencies
[ ] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
[ ] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
[ ] If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
References
Major issue related to this PR is #835 Other involved issues are: #948 #864 #884 #882 #931
Before describing the approach adopted in this PR, it is useful to summarise the problems that it directly or indirectly tries to address. Having investigated them for a long time, it has been possible to trace the root causes of these problems.
Problems caused by the template/zero row of repeatable fields
Problems caused by the repeatable field without form redrawing
Problems caused by the repeatable field with redraw
Problems caused by lookup functionality
Problems caused by form redraw
At this moment all the above problems except (5), (6) and (3) (which is a side effect) are mitigated by the redraw which is extremely frequent. The more you reduce the frequency of redraw, the more these problems arise.
But it is clear that redraw in turn causes many side effects and ideally should be avoided as much as possible.
Current workaround to all this mess
Moreover: We tried to save the focused state of the section in order to postpone the form redraw which causes the emptying of populated fields and the loss of the focus field (6). The result is that sometimes the redraw is not evoked at all as the user goes from one field to another without ever leaving the form. So we decided to revert (but still in commit history).
It doesn't solve in any way (1) whose fix, as already stated, is expensive to implement and unmaintainable.
Ideal solution: remove the template row from the repeatable field control Removing the template row from the repeatable fields immediately fix (1), (2), (3). The added submission's metadata into the store reduces the redraw so that the user experience is fluid most of the time. The redraw remains in case of interaction with lookups. Moreover Implementing the lookup so that it directly patches the form could be more than welcome. Leave the redraw as the only strategy in case the server actually enriches the form with additional data. In that case a notification will be displayed to the user and it would probably be acceptable to lose the information about the touched fields and have a solution more clean and mantainable.
Checklist
yarn run lintpackage.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.