Replace submissionList with submissionsBySurveyId

[henrycatalinismith]

Detailed notes at #1958 (comment).

Before	After

This worked surprisingly easily for me. Curious if I've missed anything due to lack of deeper familiarity with the system. Open to fine-tuning details like names and style as always.

[richardolsson]

You did exactly the thing here that I imagined, but the problem is I was wrong. Or at least, I didn't realize the effect it would have on the views/lists feature. Have a look at my comments below and you will be able to see comment by comment how I discover the problem and arrive at two possible proposed solutions. 😊

I'm glad you're exploring more parts of the codebase!

[richardolsson]

In this case, the survey ID cannot come from the URL. It's also not possible to retrieve it from the column config, apparently. Will investigate and add more info in subsequent comments.

Try clicking "Show full submission" after hovering any of the values for Angela Davis in the rightmost columns here to see the problems: https://app-zetkin-fqxi0nifd-zetkin.vercel.app/organize/1/people/lists/126

[richardolsson]

This should not have to require the surveyId. I understand why it does with your refactor, but the surveyId is not necessary to load the submission from the API, only to find it in our cache, so that is a bit of a shortcoming of the cache.

I can think of two solutions:

1. Either refactor the store so that it includes both a flat list of submissions, and a list by survey ID. This is how we do it in multiple other stores. Check out the events store for example, which contains both eventList and multiple eventsBy* maps.
2. Or change useSurveySubmission to look in all the maps to find if the submission has already been loaded. Less efficient, but also less duplication of data so less error-prone probably. Will elaborate in comment on useSurveySubmission.

[richardolsson]

Instead of finding just here, we could loop over all of the submissionsBySurveyId entries and search each of them until the correct submission is found (or not found, which means it has not been loaded and submissionItem should become falsy, so that's fine).

[henrycatalinismith]

Okay I love this. Having a lot of fun with this problem! Just writing something can be a really fun way to explore these things. Having two parallel data structures seems doable. To me it also sounds like it would be a source of bugs but by the sound of it you're used to managing the pitfalls associated with that approach here.

Iterating over the submissionsBySurveyId one by one to find a submission without requiring a survey ID works fine at read time. But without a surveyId in submissionLoad we don't know which RemoteList to mark as loading when lazy-loading a submission. So I guess it's off the table.

What if we go back to one flat submissionList? Can we fix this in an even simpler way by leaving the data structure the same as before and doing submissionList.items.filter(i => i.survey.id === surveyId) in useSurveySubmissions ?

[henrycatalinismith]

Had a chance to read the implementation of loadListIfNecessary now. Think I get why my idea doesn't work: shouldLoad won't return true when we need it to that way. Looks like the only one left standing is your first option, with e.g. an addSubmissionToState function to match how addEventToState avoids having a ton of dupicated write logic all over the place.

[richardolsson]

I think loadListIfNecessary should not be relevant for the singular submission hook (which is the one we're having problems with for the pane). In that case, finding the item and using loadItemIfNecessary should be the better option. I'm pretty sure both of my options should work.

[henrycatalinismith]

Check out 76064e3! I've tried to pay very close attention to how this works in the events slice. On my first try I missed the state.submissionsBySurveyId[surveyId].loaded = new Date().toISOString(); and it went absolutely berserk reloading the submissions over and over and over. Was a memorable way to learn that particular criterion that shouldLoad checks. After that, I had to add cd50338 in order to fix a null pointer exception in the SHOW FULL SUBMISSION button on /organize/1/people/lists/126 for Angela Davis.

Feels like I might either be missing one or two important details here, or at least not quite following the existing patterns completely faithfully, or maybe both. Let me know! I gotta run and get on with laundry day now!

[richardolsson]

This looks all correct to me! The structure of the store is not ideal, with the duplication of data, and there is indeed a risk for bugs there. We need to build some well tested utility functions to use in these situations. But that's outside the scope of this issue. I'm happy with this.

I'm going to go ahead and squash and merge!