Add automatic cleanup of local storage for saved forms of evaluations

[abc013]

This PR adds an automatic cleanup of the localStorage for forms of evaluations that have ended more than a year ago. The automatic cleanup happens when opening the student index page. We check localStorage for our entries, check the when the corresponding evaluations have ended and then remove the old entries. I implemented a POST request for fetching the evaluation end date based on id.

The commit additionally includes cleanup for other localStorage parameters when completing the evaluation.

Closes #2448.

[niklasmohrin]

I am not sure that I like the approach here, we are on the one hand forcing every user of localStorage to adhere to the key names given by localStorageConstants, but on the other hand we need to know exactly how the access of each component works internally when trying to figure out how to decode an evaluation id out of a storage key. So we are sharing implementation details in both directions here.

I think we should abstract away the localStorage access behind some interface, and then use this interface wherever we currently access localStorage. Then, an implementation of the interface can handle the expiry. So using this interface could look something like

import { ExpiringStorage, getEvaluationEnd } from "./expiring-storage.js";

const storage = ExpiringLocalStorage.localStorage(); // use localStorage as backing storage; other store usable for testing
storage.insert(getEvaluationEnd(evaluation_id), key, value);
console.log(storage.get(key));

// in base_template.ts or so
ExpiringLocalStorage.localStorage().removeExpiredEntries();

This will complicate usages though, for example in AutoFormSaver. So let's do something different.

We could instantiate the ExpiringLocalStorage with a fixed expiry time that is used whenever something is inserted using this instance. Then, instantiating one in the student_vote site would use whatever evaluation we are currently looking at, and other uses of localStorage could reuse the interface and use a constant duration into the future or so. Then, our ExpiringStorage would just mimic the interface of localStorage. I think that would be very nice.

One possible way to actually implement this would be to have two entries in the underlying storage like data_${key} and expiry_${key}. Of course, this is somewhat less accurate than your current approach in the case that an evaluation end is changed. However, I think that it is probably fine if we set the expiry date to be like 3 months or so after the suspected end.

One question that I am not sure about yet is whether such an ExpiringLocalStorage should update the expiry time of its entries when just using .get(). Both ways have some pros and cons.

What do you think? (by the way, using this set-expiry-at-insert-time approach, we would also not need the get_end_date view anymore)

cc @richardebeling

[abc013]

Hm, this would indeed be way more clean! I think it doesn't hurt to update the expiry when using get.

Before trying to implement this, is there anything else i should consider?

[richardebeling]

• +1 for a single place (i.e., typescript class) that handles expiry
• +1 for having the interface be a drop-in replacement for localstorage
• +1 for not requiring extra requests to figure out if something expired.

Determining an expiry-date when storing a value seems natural to me, we can use a very pessimistic expiry date that we extend when we update a value, my initial thought would have been to just use "1 year into the future" and not make it evaluation-dependent. Making it evaluation-dependent is also fine with me, but we'll still need buffer time.

• for data handling / privacy, active deletion as soon as its not required anymore would be optimal, but we can't fully know for sure until after the server told us that it's not required anymore, and even then evaluations can go back into in-evaluation, so anything more eager always risks data loss
• downside of later deletion is that sensitive data possibly remains in the localstorage for longer. But maybe this is two separate problems. 1 year retention period, I think, will be good enough to not over-use localstorage. For the sensitive data issue, maybe we can solve that using cryptography instead

I would try to keep it as simple and minimal as possible, in general

[niklasmohrin]

downside of later deletion is that sensitive data possibly remains in the localstorage for longer

I mean we would still use AutoFormSaver which eagerly deletes localstorage entries once a vote was successfully submitted; any case where the new approach would keep data around for a long time is equivalent to one today where we keep the same data indefinitely.

I would try to keep it as simple and minimal as possible, in general

Agree, I think that adding this feature today with 1 year expiry is a solid improvement, we can think about choosing better dates in a follow up.

[niklasmohrin]

I think the canonical form I would expect is something like

GET /student/evaluation_end_date?evaluation_id=xxx

[niklasmohrin]

This is however the case for evaluations that users have successfully voted for, so PermissionDenied seems a bit off. I think we can relax the restriction here to request.user in evaluation.participants.all()

[richardebeling]

Uh, one important additional aspect here: If we change anything around what local storage entries look like, we have to consider how to migrate.

For example, the data_${key} and expiry_${key} scheme proposed above by @niklasmohrin would need some conversion of existing entries. I think I wouldn't put it in two entries, but rather make it a wrapping thing, so that we have ${key} = {"expiry": "...", payload: "..."} and the accessor-class unwraps this, but both need a migration.

Maybe we can find a time in the year/semester where we don't have any open evaluations, then we can just migrate at that point, but syncing that with the merge-time could be annoying.

Maybe, this is an argument for not changing existing entries, but using a scheme of localstorage entries ${key} and ${key}_expiry. Haven't made up my mind about this yet

[niklasmohrin]

Fair, I guess wrapping into JSON makes sense. I don't think key and key_expiry would work as key could conflict with the _expiry pattern.

For migrating, we could check whether an accessed entry follows the JSON scheme we expect, and if not, convert it to that. To be sure about what our entries are, we could also always include a key like evap-localstorage-scheme-version: 1 or so - this would also allow us to migrate further if we ever need to.

Alternatively, I could see that we have a setting that enables the new interface and toggle it in the semester when we think it makes sense.

I think I prefer the former though

[niklasmohrin]

@abc013 Do you want to continue working on this?

[abc013]

@niklasmohrin yes, it's just all the way down in my priority list. I pushed it a little bit up now.

[richardebeling]

@abc013 that's ok, don't stress

[karyon]

I think this would be much better solved in a much smaller way, or not at all.

Let's assume we clear the language setting on submit, which we should be doing in any case.

Consider the proposed ExpiringLocalStorage. It would need a bunch of feature code, migration code, ideally unit tests covering the ExpiringLocalStorage itself as it governs all writing and deleting of rather important data, or actually ideally an end-to-end test for deleting the form data since we'd never notice if this breaks, and probably some maintenance in the future.

The alternative would be to do nothing. Effectively, this leaves the questionnaire language setting in storage indefinitely if the user changed the language, and same for answers if the users entered any, and this only if the user never submits their vote. The amount of data is negligible. I think the only argument would be that the user might enter sensitive data (or we consider any answers sensitive), forget to (or choose to not) submit them, and we would like to clear those from the user's own device. After a year, mind you, which is a long time in which bad things can happen with sensitive data, but chances are the user has actually switched devices, or browsers, or cleared their browsing data in the meantime.

I think this is not worth it and I would avoid adding any complexity for that, in particular to the much more important path that saves the form data.

A smaller alternative would be to create a list of archived (or published) evaluations in student.views.index, put them in an array in the template, and then delete all student-vote-language-XYZ and student-vote-last-saved-at-XYZ localstorage entries that are contained there. This would be much less code, wouldn't wrap all storage operations, and also potentially delete entries much earlier than a year. But, again, I would suggest to not solve this at all.