#2350: Generate survey files async #2362

anaPerezGhiglia · 2024-08-05T18:22:46Z

Changes

create new SurveyResults GenServer
move responsibility of generating files from RespondentController to new SurveyResults actor
change files endpoints to "common" POST (without csv format)
change UI to make the POST call instead of a redirect to a new location

Out of scope

Keep track of generated files in the db
Files cleanup policy
Download existing files

for #2350

matiasgarciaisaia · 2024-11-13T21:00:45Z

First of all, sorry for the huge PR. Things got wild.

Here's my list of pending things, which we may want to move to a different issue/PR.

Bug: not sending the filter to the file status request
Improve downloads modal UX (prevent generating a file that's being generated, show the user a file is currently being generated, add an exponential backoff to check the files status after triggering a file generation)
Filtered results files should consider the filter (ie, don't show a disposition:completed file if filtering for disposition:ready).
Check Survey state against pre-generated files (eg, don't offer to download a file from when the survey was running if it's already finished, maybe prevent regenerating files from finished surveys)
Only return status of files that the user can access (we're leaking some harmless info there - files can't be downloaded by unauthorized users)
Avoid concurrent fetchRespondentsFilesStatus calls
Fix skipped tests

I'll try to address some of these while we review the PR.

instead of Questionnaire

See #2350

We should un-skip them by the end of the PR. See #2350

The info about the generated files is still pending. See #2350

See #2350

This will allow us to check if there already is a file generated or not. Also, we move the decision of whether to regenerate a file or not to the user (instead of checking if we should generate the file again or not). See #2350

From the UI, request that the CSV files are generated by the backend. We still miss checking if the files exist or are currently being generated. See #2350

See #2350

Small changes, nothing too relevant. See #2350

There's probably still an issue with react-timeago not being properly ignored yet. See #2350

There are no definitions available. See #2350

Thanks, eslint! See #2350

They've already been solved or there's nothing to do about them.

See #2350

They were previously placed in a public directory, statically served. We now apply the usual access control we have for generation (and had for streaming). See #2350

Those URLs in the RespondentIndex.jsx smelled. See #2350

Due to the way the `q` variable was bound in the previous implementation of the `setInterval` call, we were always querying the file status using the filter (or lack thereof) that was in place in the first show of the modal. We now get the current value of `q` at the time in which the refresh function is called. See #2350

They were already working - not sure when they were fixed but that's fine. See #2350

We were raising an exception instead. See #2350

The URLs where broken when splitting generation and download. See #2350

matiasgarciaisaia · 2024-11-28T21:17:38Z

Surveda won't manage old survey files (they'll pile up in the FS)
We should probably Docker-mount another volume/FS in the directory where the files are going to be generated in
Filtered results should honour the current filter (we're serving whichever filter we last generated). For this to work, filtered results should include filter in filename (beware special characters like : and ).

See #2350

anaPerezGhiglia

I cannot officially approve this PR since I was the original author, but this looks good to me! ✨
@matiasgarciaisaia I would encourage we deploy a preview of this branch to staging so we can play with it a bit without having to set it up in each of our local environments 😬

assets/js/actions/survey.js

anaPerezGhiglia · 2024-12-04T20:48:56Z

lib/ask_web/controllers/respondent_controller.ex


-    ActivityLog.download(project, conn, survey, "survey_results") |> Repo.insert()
+    ActivityLog.generate_file(project, conn, survey, "survey_results") |> Repo.insert()


following the naming normalization perhaps we can change the ActivityLog entry to register respondents_results instead of survey_results

I wanted to keep the files themselves backwards-compatible. Changing these names would imply running a migration to change old activity logs from the DB. Which could be OK, but these are not exactly the same strings used "internally". We should also take a look at including the filter, somehow, for the results file - here we're completely ignoring whether there's a filter or not.

anaPerezGhiglia · 2024-12-04T20:50:35Z

lib/ask_web/controllers/respondent_controller.ex

+
+    SurveyResults.generate_disposition_history_file(survey.id)
+
+    ActivityLog.generate_file(project, conn, survey, "disposition_history") |> Repo.insert()


should we create en enum or constants with the different file names so we avoid repeating the strings all over?

lib/ask/survey_results.ex

See #2362 See #2350

Without invalidating the timer, navigating from one survey to another would keep the status for both surveys' files to be fetched, yielding bugs in the UI. See #2350 See #2362

See #2350

anaPerezGhiglia

With the last changes the files modal behaves as expected - even changing between different surveys. ✅
However, what I've noticed is that once the user opens the "download files" modal, the app keeps on requesting the /files status - even if the user has closed the modal. This is not critical, but it's useless. Even more because if the user opens the modal again, a new request is triggered

matiasgarciaisaia · 2024-12-10T14:48:53Z

Yes. We don't have (or I haven't found) an event for when the modal closes, so the requests keep going every 20 seconds even when we don't need them.

matiasgarciaisaia force-pushed the feature/2350-survey-file-manager branch from c9ee306 to 02a9384 Compare August 15, 2024 22:42

matiasgarciaisaia force-pushed the feature/2350-survey-file-manager branch from f08c2b8 to b633f1a Compare November 13, 2024 20:53

matiasgarciaisaia marked this pull request as ready for review November 13, 2024 20:57

manumoreira requested review from ggiraldez, ysbaddaden and mverzilli November 14, 2024 12:33

matiasgarciaisaia force-pushed the feature/2350-survey-file-manager branch from 06aad77 to a12192f Compare November 14, 2024 18:00

anaPerezGhiglia and others added 22 commits November 14, 2024 15:01

create SurveyFilesManager actor to handle generation of files

100784f

Generate interactions file into a new dir

1539c01

add basic decision for generating file or not

aa163c8

Add incentives file to SurveyFilesManager

d39129e

add disposition_history files to SurveyFilesManager

c4c22ef

Convert csv trigger request into POST requests with no csv format

3a3d084

add respondents_result file to SurveyFilesManager

d1f861b

Cleanup

400135d

Fix router path to point to new get_respondents_result path

45b1181

Remove respondents_where from Survey model

59f4893

Rename Ask.Runtime.SurveyFilesManager to Ask.SurveyResults

bba89be

Move all_questionnaires_fields to SurveyResults

65e6f12

instead of Questionnaire

remove unused import

2017114

Scaffold Survey Results tests

1264365

See #2350

More tests

346e866

Keep moving tests

2244b16

See #2350

Make test suite "pass" by skipping broken tests

2cc0605

We should un-skip them by the end of the PR. See #2350

File downloads UI - first take

744e17a

The info about the generated files is still pending. See #2350

Stub downloading previously generated result files

1f50e33

See #2350

Typo

af1a87e

See #2350

Generate async files in a stable path

8d0f518

This will allow us to check if there already is a file generated or not. Also, we move the decision of whether to regenerate a file or not to the user (instead of checking if we should generate the file again or not). See #2350

Trigger files generation

17496f6

From the UI, request that the CSV files are generated by the backend. We still miss checking if the files exist or are currently being generated. See #2350

matiasgarciaisaia added 17 commits November 14, 2024 15:01

Add file status endpoint

c76fada

See #2350

Make API return a map of files instead of an array

0d545fd

See #2350

Fetch files status upon Downloads dialog load

8ef0ffd

See #2350

Fetch files status upon download dialog

1b17181

See #2350

Enable/disable file buttons depending on state

b14a27a

See #2350

Regularly fetch respondent files status

d3e1923

See #2350

Fix SurveyResults tests

1b7d329

Small changes, nothing too relevant. See #2350

Fixing FlowJS typing

2dd4b58

There's probably still an issue with react-timeago not being properly ignored yet. See #2350

Ignore Flow-typing react-timeago

bacac9c

There are no definitions available. See #2350

Remove unused variables

2526d5e

Thanks, eslint! See #2350

Drop old FIXMEs

b8fd21c

They've already been solved or there's nothing to do about them.

Log file download vs generation in ActivityLog

e02a40b

See #2350

Style Download CSV files modal

d95502b

See #2350

Access control generated CSV files

d3e967d

They were previously placed in a public directory, statically served. We now apply the usual access control we have for generation (and had for streaming). See #2350

Move API routes concerns back to api.js

b04f065

Those URLs in the RespondentIndex.jsx smelled. See #2350

Unskip survey links tests

d5ceb9a

They were already working - not sure when they were fixed but that's fine. See #2350

matiasgarciaisaia force-pushed the feature/2350-survey-file-manager branch from a12192f to d5ceb9a Compare November 14, 2024 18:04

matiasgarciaisaia added 2 commits November 14, 2024 17:43

Respond a 404 for files not yet generated

6c1d42e

We were raising an exception instead. See #2350

Fix: short links point to the right file downloads

c9ba84b

The URLs where broken when splitting generation and download. See #2350

matiasgarciaisaia force-pushed the feature/2350-survey-file-manager branch from 7aeabb7 to c9ba84b Compare November 14, 2024 20:43

Honour results filter when generating file

63a3d36

See #2350

anaPerezGhiglia commented Dec 4, 2024

View reviewed changes

matiasgarciaisaia added 3 commits December 5, 2024 19:12

Nitpick: renames and comments from the PR

b3414c3

See #2362 See #2350

Fix: avoid fetching file status after navigating

6ad6f92

Without invalidating the timer, navigating from one survey to another would keep the status for both surveys' files to be fetched, yielding bugs in the UI. See #2350 See #2362

Mouse pointer on file generation button

9b2b9c5

See #2350

anaPerezGhiglia commented Dec 10, 2024

View reviewed changes

matiasgarciaisaia merged commit d1b78b1 into main Dec 17, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2350: Generate survey files async #2362

#2350: Generate survey files async #2362

anaPerezGhiglia commented Aug 5, 2024 •

edited by matiasgarciaisaia

Loading

matiasgarciaisaia commented Nov 13, 2024 •

edited

Loading

matiasgarciaisaia commented Nov 28, 2024 •

edited

Loading

anaPerezGhiglia left a comment

anaPerezGhiglia Dec 4, 2024

matiasgarciaisaia Dec 5, 2024

anaPerezGhiglia Dec 4, 2024

anaPerezGhiglia left a comment

matiasgarciaisaia commented Dec 10, 2024


		ActivityLog.download(project, conn, survey, "survey_results") \|> Repo.insert()
		ActivityLog.generate_file(project, conn, survey, "survey_results") \|> Repo.insert()


		SurveyResults.generate_disposition_history_file(survey.id)

		ActivityLog.generate_file(project, conn, survey, "disposition_history") \|> Repo.insert()

#2350: Generate survey files async #2362

#2350: Generate survey files async #2362

Conversation

anaPerezGhiglia commented Aug 5, 2024 • edited by matiasgarciaisaia Loading

Changes

Out of scope

matiasgarciaisaia commented Nov 13, 2024 • edited Loading

matiasgarciaisaia commented Nov 28, 2024 • edited Loading

anaPerezGhiglia left a comment

Choose a reason for hiding this comment

anaPerezGhiglia Dec 4, 2024

Choose a reason for hiding this comment

matiasgarciaisaia Dec 5, 2024

Choose a reason for hiding this comment

anaPerezGhiglia Dec 4, 2024

Choose a reason for hiding this comment

anaPerezGhiglia left a comment

Choose a reason for hiding this comment

matiasgarciaisaia commented Dec 10, 2024

anaPerezGhiglia commented Aug 5, 2024 •

edited by matiasgarciaisaia

Loading

matiasgarciaisaia commented Nov 13, 2024 •

edited

Loading

matiasgarciaisaia commented Nov 28, 2024 •

edited

Loading