Auto-save in new Item Submission form breaks the form

[alemarte]

References

Major issue related to this PR: Fixes #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

  • (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. Required repeatable fields get validated incorrectly in submission form #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. During submission some Repeatable fields stop working after a few inserts #948

• Problems caused by the repeatable field with redraw

  • (3) The content in the template/zero row is deleted. Usability issue with submission's repeatable fields #864 Cannot complete a new submission on demo site or locally (when forgetting to click "+" next to repeatable field) #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 maintainable.

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.
• 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.

[lgtm-com]

This pull request introduces 2 alerts when merging 4a1c1bf into aa8feb0 - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class

[artlowel]

Thanks @alemarte!

Everything seems to work well, barring the exceptions you mention in the description.
I do have some inline code comments though.

A note to other reviewers: autosave is disabled by default you need to set environment.submission.autosave.timer to something higher than 0. Also note that the unit is minutes not milliseconds like other periods in the environment file

[tdonohue]

@alemarte : I gave this a review & test as well. First, the code looks good overall, but I agree with all the feedback that @artlowel gives above (so those areas should be resolved). I added one general comment inline that I think the timer should be converted to milliseconds -- but that could be done in a follow-up PR (not required here, but it looks like a small change to me).

I tested this by running yarn start and setting the timer: 1 (one minute). It mostly works, but I noticed the following oddities:

1. First, if I click around in the submission form (I think it happened when I clicked into the Abstract field), I occasionally see this error appear in Chrome DevTools. This error doesn't seem to affect the submission form, but it should be fixed if possible:

   ERROR TypeError: Cannot read property 'includes' of undefined
    at section-form.component.ts:230
    at Array.forEach (<anonymous>)
    at l.hasMetadataEnrichment (section-form.component.ts:229)
    at l.updateForm (section-form.component.ts:294)
    at t._next (section-form.component.ts:352)
    at t.__tryOrUnsub (Subscriber.js.pre-build-optimizer.js:194)
    at t.next (Subscriber.js.pre-build-optimizer.js:132)
    at t._next (Subscriber.js.pre-build-optimizer.js:76)
    at t.next (Subscriber.js.pre-build-optimizer.js:53)
    at t._next (distinctUntilChanged.js.pre-build-optimizer.js:55)
   

2. I noticed that when the auto-save triggers, I no longer see a notification that the form was saved (in the upper right). I'm assuming that was removed on purpose? I'm OK with it's removal if it caused issues, I just was surprised by it.
3. Finally, the first form section never shows the green checkbox unless I enter a name in the "template row" (first row). Here's how I triggered that:
   • I opened the form,
   • I first entered a name, clicked "Add".
   • Then entered a Title & Date
   • I still see an orange exclamation mark (even though all required fields now have values).
   • If I go back to the Author field and typed something in the "Last Name" of the template row, then it immediately turned into a green checkbox.
4. Strangely, I'm also occasionally able to generate an invalid PATCH (HTTP 500 response). I've not yet narrowed down how to reproduce this reliably. But, it occurred when I tried to reproduce (3) above without a file attached. In that scenario, I was again able to reproduce (3) but also started getting 500 responses from the backend each time the autosave triggered. The 500 response seems to be caused by the PATCH payload is trying to "add" and "remove" the same author field multiple times in a single payload (literally the payload has "add" of an author name, then a "remove" then a second "remove" and then an "add" of the same name again). This one may be harder to reproduce though, so I'm uncertain whether it's this PR at fault.

If I turn off autosave (set timer: 0), then I no longer can reproduce (1), (2) or (4). However, (3) still seems to exist.

As most of these issues seem related to the Author field (a repeatable field), it's possible they are all related to #971 (and the issues described in this ticket description). So, I'm OK with this PR moving forward (once other feedback is resolved) since it disables autosave by default. We could move these other issues to the discussion in #971.

[tdonohue]

I would prefer that we switch this timer to milliseconds (as all other time-based configs use milliseconds instead of minutes). I see that in submission.service.ts, we already are using this timer config to calculate the milliseconds anyways.

While I realize this timer was not created by this PR, I wanted to note this preference as it seems like something that could be easily fixed in this PR & it'd align better with our other configurations. But, if others disagree, we could fix this in a future PR.

[alemarte]

@tdonohue timer has been refactored to handle milliseconds. Still investigating on the other feedbacks...

[alemarte]

1. The section metadata is computed asynchronously, so an extra check sounds reasonable in that case. I added it here
   

   dspace-angular/src/app/submission/sections/form/section-form.component.ts

   Line 243 in 8e77fac

   if (this.sectionMetadata && this.sectionMetadata.includes(key)) {

2. The notifications are displayed only on manual savings (accordingly to the wrapping up comment related to the 835)
3. This is the very big side effect of the template row of repeatable fields and is the problem I labeled with (1)
4. Is the problem I labeled with (2). The current implementation fix the 'displayed value', but internally form data structures aren't always consistent with the model they'd expect to be.

[lgtm-com]

This pull request fixes 1 alert when merging 23cb3bb into 17ca2c4 - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 042d2e7 into e867bad - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 8e77fac into e867bad - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 1 for Unused variable, import, function or class

[alemarte]

@artlowel @tdonohue I've addressed all your feedbacks. I'll comment all of them in a minute.
Before doing that I have to warn you about a force push that unfortunately I had to perform. I screw up a bit during a rebase with the latest from main to merge a couple of conflicts, very simple indeed. I don't have anymore my local before merging so I had to rebase the only one I had after rebasing. If you have locally the previous copy of the branch you can check that differences are minimal. With this last commit I cleanup and add a missing test for a method.

[lgtm-com]

This pull request fixes 1 alert when merging 6cf6dee into e867bad - view on LGTM.com

fixed alerts:

• 1 for Unused variable, import, function or class

[artlowel]

Thanks @alemarte!

[tdonohue]

👍 Looks good to me now, @alemarte ! Your feedback makes sense, and (as you noted) several of my previously reported issues will be resolved in #971 (which is a follow-up ticket to this one).

Therefore, I'm OK with merging this as-is, since it definitely improves the behavior of auto-save. Further fixes/improvements will be made in the solution to #971 (see that ticket for more details).