Skip to content

Add automatic cleanup of local storage for saved forms of evaluations #2528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
abc013 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add automatic cleanup of local storage for saved forms of evaluations #2528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions evap/static/ts/src/auto-form-saver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ New portions and modifications are licensed under the conditions in LICENSE.md

import { assert } from "./utils.js";

type LocalStorageConstantsDictionary = Record<string, string>;
export const localStorageConstants: LocalStorageConstantsDictionary = {
"local-storage-key-last-saved-at": "student-vote-last-saved-at",
"local-storage-key-language": "student-vote-language",
"local-storage-key-form-data": "student-vote",
};

interface AutoFormSaverOptions {
href: string;
customKeySuffix: string;
Expand Down Expand Up @@ -80,6 +87,10 @@ export class AutoFormSaver {

getPrefix(field: Element) {
return (
localStorageConstants["local-storage-key-form-data"] +
"-" +
this.options.href.split("/").slice(-1)[0] +
"-" +
this.options.href +
this.target.id +
this.target.name +
Expand Down
64 changes: 64 additions & 0 deletions evap/student/templates/student_index.html
View file
Open in desktop
Copy link
Copy Markdown
Member

@niklasmohrin niklasmohrin Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

@abc013 abc013 Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@richardebeling richardebeling Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@niklasmohrin niklasmohrin Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@richardebeling richardebeling Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@niklasmohrin niklasmohrin Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{% extends 'base.html' %}

{% load static %}
{% load infotext_templatetags %}
{% load evaluation_filters %}

Expand Down Expand Up @@ -31,3 +32,66 @@
{% endif %}
{% include 'student_index_semester_evaluations_list.html' %}
{% endblock %}

{% block additional_javascript %}
{% if not preview %}
<script type="module">
import { assert } from "{% static 'js/utils.js' %}";
import { CSRF_HEADERS } from "{% static 'js/csrf-utils.js' %}";
import { localStorageConstants } from "{% static 'js/auto-form-saver.js' %}";

var evaluationIdDictionary = {};
for (var i = 0; i < localStorage.length; i++){
var key = localStorage.key(i);
var evaluationId = null;
if (key.startsWith(localStorageConstants["local-storage-key-last-saved-at"])) {
var shortenedKey = key.replace(localStorageConstants["local-storage-key-last-saved-at"], "");
evaluationId = shortenedKey.split("-")[1];
} else if (key.startsWith(localStorageConstants["local-storage-key-language"])) {
var shortenedKey = key.replace(localStorageConstants["local-storage-key-language"], "");
evaluationId = shortenedKey.split("-")[1];
} else if (key.startsWith(localStorageConstants["local-storage-key-form-data"])) {
var shortenedKey = key.replace(localStorageConstants["local-storage-key-form-data"], "");
evaluationId = shortenedKey.split("-")[1];
}

if (evaluationId === null) {
continue;
}

if (evaluationIdDictionary[evaluationId] === undefined) {
evaluationIdDictionary[evaluationId] = [];
}
evaluationIdDictionary[evaluationId].push(key);
}

async function fetchEndDate(evaluationId) {
try {
const response = await fetch("{% url 'student:get_end_date' %}", {
body: new URLSearchParams({ evaluation_id: evaluationId }),
headers: CSRF_HEADERS,
method: "POST",
});
assert(response.ok);
return response.text();
} catch (err) {
console.error(err);
window.alert("The server is not responding, or invalid response.");
}

return Date.now();
}

for (var evaluationId in evaluationIdDictionary) {
const endDate = new Date(Date.parse(await fetchEndDate(evaluationId)));
const distance = Date.now() - endDate;
if (distance >= 365 * 24 * 60 * 60 * 1000) {
console.log("The evaluation period for " + evaluationId + " has ended more than a year ago, removing data from local storage. ");
for (var key of evaluationIdDictionary[evaluationId]) {
localStorage.removeItem(key);
}
}
}
</script>
{% endif %}
{% endblock %}
12 changes: 9 additions & 3 deletions evap/student/templates/student_vote.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,20 +228,20 @@ <h3 class="mb-3">
{{ text_answer_warnings|text_answer_warning_trigger_strings|json_script:'text-answer-warnings' }}

<script type="module">
import { AutoFormSaver } from "{% static 'js/auto-form-saver.js' %}";
import { AutoFormSaver, localStorageConstants } from "{% static 'js/auto-form-saver.js' %}";
import { CSRF_HEADERS } from "{% static 'js/csrf-utils.js' %}";
import { initTextAnswerWarnings } from "{% static 'js/text-answer-warnings.js' %}";
import { assert } from "{% static 'js/utils.js' %}";

const lastSavedStorageKey = "student-vote-last-saved-at-{{ evaluation.id }}-{{ request.user.id }}";
const lastSavedStorageKey = localStorageConstants["local-storage-key-last-saved-at"] + "-{{ evaluation.id }}-{{ request.user.id }}";
const textResultsPublishConfirmation = {
top: document.querySelector("#text_results_publish_confirmation_top"),
bottom: document.querySelector("#text_results_publish_confirmation_bottom"),
bottomCard: document.querySelector("#bottom_text_results_publish_confirmation_card"),
};

// Ensure that selected questionnaire language is saved and loaded
const languageStorageKey = "student-vote-language-{{ evaluation.id }}-{{ request.user.id }}";
const languageStorageKey = localStorageConstants["local-storage-key-language"] + "-{{ evaluation.id }}-{{ request.user.id }}";
const params = new URLSearchParams(document.location.search);
const currentlySelectedLanguage = "{{ evaluation_language|escapejs }}";
const savedLanguage = localStorage.getItem(languageStorageKey);
Expand Down Expand Up @@ -340,6 +340,12 @@ <h3 class="mb-3">
return response.text();
}).then(response_text => {
if(response_text === "{{ success_magic_string }}") {
if (localStorage.getItem(languageStorageKey) !== null) {
localStorage.removeItem(languageStorageKey);
}
if (localStorage.getItem(lastSavedStorageKey) !== null) {
localStorage.removeItem(lastSavedStorageKey);
}
formSaver.releaseData();
window.location.replace("{{ success_redirect_url }}");
} else {
Expand Down
1 change: 1 addition & 0 deletions evap/student/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

urlpatterns = [
path("", views.index, name="index"),
path("get_end_date", views.get_end_date, name="get_end_date"),
Copy link
Copy Markdown
Member

@niklasmohrin niklasmohrin Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the canonical form I would expect is something like

GET /student/evaluation_end_date?evaluation_id=xxx

path("vote/<int:evaluation_id>", views.vote, name="vote"),
path("drop/<int:evaluation_id>", views.vote, {"dropout": True}, name="drop"),
]
10 changes: 10 additions & 0 deletions evap/student/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from django.utils import translation
from django.utils.translation import get_language
from django.utils.translation import gettext as _
from django.views.decorators.http import require_POST

from evap.evaluation.auth import participant_required
from evap.evaluation.models import (
Expand Down Expand Up @@ -388,3 +389,12 @@ def vote(request: HttpRequest, evaluation_id: int, dropout: bool = False) -> Htt

messages.success(request, _("Your vote was recorded."))
return HttpResponse(SUCCESS_MAGIC_STRING)

@require_POST
def get_end_date(request):
evaluation = get_object_or_404(Evaluation, id=request.POST["evaluation_id"])

if not evaluation.can_be_voted_for_by(request.user):
Copy link
Copy Markdown
Member

@niklasmohrin niklasmohrin Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

raise PermissionDenied

return HttpResponse(evaluation.vote_end_date.isoformat())
Loading