Review update mechanisms: triggers, queue and local storage

[timgdavies]

In addition to the locking issues in #26 we are experiencing a few unexplained bugs leading to:

Duplicated Entries

Expected behaviour: The frontend application fetches the questions and answers. Where an answer already exists, then changes to that question should trigger updateRow using the ID of the entry in the database to update the record.

Observed behaviour: In some cases, we appear to have bulk writes coming through the API to Insert new blank records for questions that already had answers. This creates duplicate survey_data entries for a given question, and the blank one with a higher ID ends up returned when the survey is next loaded - appearing to create data loss to the user.

Notably, the later entries appear to include no data, but include the privatenotes and justification fields (even when these are not enabled for the question).

Earlier entry:

{'example0': '', 'example1': '', 'example2': '', 'example3': '', 'example4': '', 'response': '2', 'questionid': 'G.GOVERNANCE.DPL.e.e1.BREACH'}

Later 'duplicate' (Inserted rather than updated):

{'example0': '', 'example1': '', 'example2': '', 'example3': '', 'example4': '', 'response': '', 'confidence': '', 'questionid': 'G.GOVERNANCE.DPL.e.e1.BREACH', 'privatenotes': '', 'justification': ''}

(Current mitigation: We have a management script that looks for these cases, and removes the later duplicate entries - but it has to be run manually).

Possible explanations:

• We are not sure what is causing this to happen. Areas to investigate may be:
  • Are there times when the survey is failing to load existing answers and then triggering an update on lots of fields - or triggering update on lots of fields before answer data has been properly loaded?

Removed responses

Expected behaviour: Once set, the response to a question should only be changed by user input.

Observed behaviour: We are seeing surveys where at the same time, an update takes place which sets a large number of questions responses to blank.

Example from our debug script for this:

The older log entry (242128) for question U.COMPANY.DUEDIL.e.e2.AI from 2021-07-21 07:11:16.545850+00:00 has a response of 2 whereas the later log entry (242702) from 2021-07-21 07:58:34.186739+00:00 does not.

Replace: 
 {'id': 27804, 'data': {'example0': '', 'example1': '', 'example2': '', 'example3': '', 'example4': '', 'response': '', 'questionid': 'U.COMPANY.DUEDIL.e.e2.AI'}, 'type_id': 2, 'survey_id': 171} 

 with:
{'id': 27804, 'data': {'example0': '', 'example1': '', 'example2': '', 'example3': '', 'example4': '', 'response': '2', 'questionid': 'U.COMPANY.DUEDIL.e.e2.AI'}, 'type_id': 2, 'survey_id': 171}

What we need to do

Investigate what is causing these issues and aim to put fixes in place.

[timgdavies]

We suspect the Local Storage process is the main cause of Duplicated Entries and Removed Responses in the scenario where:

• Insert or Update request containing blank data get stuck in local storage (potentially triggered when a researcher is first looking through the survey and clicking through elements, or from some other process that triggers the Angular update for a large set of survey questions)
• Other real research edits happen and get written to the API
• At some future point, when the application is refreshed, the earlier Inserts or Updates are fetched from local storage and written to the queue, and then sent to the API, overwriting earlier data.

Possible mitigations

(1) Remove local storage

In this scenario, we need to make sure that when a write to the API fails:

(a) The user is warned that the connection to the server has been lost;
(a) If data remains in the queue when a user is trying to close the page, they are warned that navigating away from the page could lead to a loss of data;

(2) Handle for field-level 'conflicts' in some way

In this scenario, we would introduce some sort of field-level locking, so that each field on the server-side has a timestamp for when it was last edited.

• I.e. The answer data model in angular would have a 'timestamp' which is updated on the client side whenever the answer is updated;
• This would be written to the API, so that the data in Postgres contains the timestamp of the last update
• If an update is written that has an earlier timestamp than the current postgres version, the update would be rejected (And the user asked to refresh their local copy to get the latest data from the server)

(Note: we would need to handle for misconfigured clocks on local machines)

(3) Introduce more error checking on the server side

The server could reject inserts with duplicate question IDs for a given survey, and writes that contain blank responses (we would need to have a better way of 'unsetting' an answer in this case - which currently takes place by turning it blank)

Investigation needed

How does the client side application currently handle the case where the API:

• Times out on an insert or update request;
• Returns an error from an insert or update request;

@aliblackwell can you explore - and see which of the options above you think looks best / or consider other options here.

[timgdavies]

Another option might be to add a check when retrieving something from local storage to be sure that it contains data.

The common property of our duplicated entries and removed responses is generally than all fields are null. So - if when getting data out of local storage we drop anything with all empty fields, we could have a 'fix', if not full solution.

[aliblackwell]

Hi @timgdavies I have investigated and also attempted to reproduce #29. I couldn't, however I did notice that the localStorage queue is being populated whether or not you make changes, and these are then sent up to the server as if they were genuine original researcher edits. It is my view that the localStorage feature is buggy and causing these issues, and we should therefore remove it - I don't believe the benefits a working implementation would bring outweigh the development costs of fixing it.

I have submitted a PR (#31) that removes it and also implements 1a)ii from above:

If data remains in the queue when a user is trying to close the page, they are warned that navigating away from the page could lead to a loss of data;