Skip to content
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.

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

Survey submissions show up in wrong survey #1958

Closed
richardolsson opened this issue May 15, 2024 · 2 comments · Fixed by #1971
Closed

Survey submissions show up in wrong survey #1958

richardolsson opened this issue May 15, 2024 · 2 comments · Fixed by #1971
Labels
🐜 bug Something isn't working 🚪 entry-level Good for newcomers 🐬 Medium Just a nice sized issue.

Comments

@richardolsson
Copy link
Member

Description

When navigating around between surveys, without ever refreshing the page, there seems to be something wrong with the cache that is causing submissions to be rendered in the list for the wrong survey.

Steps to reproduce

  1. Go to "My very open survey" at https://app.dev.zetkin.org/organize/1/projects/standalone/surveys/1/submissions
  2. Note the survey submissions in the list
  3. Click on "Projects" in the breadcrumbs
  4. Click on the "Activities" tab
  5. Find any survey other than "My very open survey" and click on it
  6. Click the "Submissions" tab

Expected Behaviour

The list should contain other submissions than the one listed in step 2.

Actual Behaviour

The list is identical to the one in step 2, showing submissions of another survey than the one we are visiting. Refreshing the page shows the correct results.

Screenshots (if you have any)

Original survey

image

Another survey (showing same submissions)

image

@richardolsson richardolsson added 🐜 bug Something isn't working 🚪 entry-level Good for newcomers 🐬 Medium Just a nice sized issue. labels May 15, 2024
@richardolsson
Copy link
Member Author

This must be a bug with how data is cached in the store. When submissions have already been loaded in one survey, visiting the submissions list for another survey does not trigger submissions to be loaded anew, but just uses the same cached list.

Without having looked at this yet, I would recommend that whoever takes this on starts by looking at the structure of the survey store. Submissions should be stored per survey (e.g. submissionsBySurveyId). It makes no sense to have a single submissions list.

@henrycatalinismith
Copy link
Collaborator

Continuing to focus on mopping up survey issues ahead of the big merge. Best to provide a nice clear landing zone for that thing. So I've been investigating your suggested fix here and there's even a // TODO: Segregate submission content from submission list just like your comment.

From there I've been looking at everything that either reads from or writes to submissionList. Each such place will need updating. Each one will now need a survey ID, and in some cases this will mean changing call signatures for store actions, hooks, and maybe components too. Nothing unusual here, just me mapping out the solution verbally.

I wanted to get a real clear understanding of the layout of these relationships before even beginning to code. So here's a flow diagram representing the network of relationships between everything connected to the submissionList property in the survey slice.

flowchart LR

  subgraph store
    subgraph state
      submissionList
    end

    subgraph actions
      submissionLoad
      submissionLoaded
      surveySubmissionUpdate
      surveySubmissionUpdated
      surveySubmissionsLoad
      surveySubmissionsLoaded
    end
  end

  submissionLoad --> submissionList
  submissionLoaded --> submissionList
  surveySubmissionUpdate --> submissionList
  surveySubmissionUpdated --> submissionList
  surveySubmissionsLoad --> submissionList
  surveySubmissionsLoaded --> submissionList

  subgraph hooks
    useHydratedSurveySubmission
    useSurveySubmission
    useSurveySubmissions
  end

  useHydratedSurveySubmission --> useSurveySubmission
  useSurveySubmission --> submissionList
  useSurveySubmission --> submissionLoad
  useSurveySubmission --> submissionLoaded
  useSurveySubmission --> surveySubmissionUpdate
  useSurveySubmission --> surveySubmissionUpdated
  useSurveySubmissions --> submissionList
  useSurveySubmissions --> surveySubmissionsLoad
  useSurveySubmissions --> surveySubmissionsLoaded

  subgraph components
    SurveyOptionColumnType
    SurveyResponseColumnType
    SurveySubmittedColumnType
    SurveySubmissionPane
    SurveySubmissionsList
  end

  subgraph pages
    submissions.tsx
  end

  submissions.tsx --> useSurveySubmissions
  SurveySubmissionsList --> useSurveySubmission
  SurveySubmissionPane --> useHydratedSurveySubmission

  submissions.tsx --> SurveySubmissionsList
  SurveyOptionColumnType --> SurveySubmissionPane
  SurveySubmissionsList --> SurveySubmissionPane
  SurveyResponseColumnType --> SurveySubmissionPane
  SurveySubmittedColumnType --> SurveySubmissionPane

I do see the hypothetical potential for an abstraction leak in SurveyOptionColumnType and friends. Those are loaded into ViewDataTable, which is used all over the place. If someone more familiar with this code can confirm that's not a risk here then the above flow chart is actually capturing the entire problem space in one neat little box.

Looks to me like this is a matter of grabbing surveyId out of useRouter().query and then passing it down through the params of the hooks and actions so that everywhere that needs it has it. Sound like a decent plan?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐜 bug Something isn't working 🚪 entry-level Good for newcomers 🐬 Medium Just a nice sized issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants