Lesson resources selection

[AlexVelezLl]

Summary

This PR:

• Proposes a new strategy to handling views and navigation in side panels:
  • Change the LESSON_SELECT_RESOURCES route from select-resources/:topicId? to select-resources/index, select-resources/bookmarks, select-resources/channels to handle the views that will be displayed during resource selection. Make the topicId be sent via query instead of params.
  • Make each "subpage" of the side panel (each snapshot that the user sees) a component that logically groups several working pieces. So if we see the index of the select resources, then we render the SelectionIndex component, if we want to see the bookmarks page we render the SelectFromBookmarks component, etc. This way we remove the complexity and poor maintainability that comes with having a bunch of conditions like !showSearch && !showBookmarks && ... etc to render each piece of the side panel.
• Implements a new general purpose composable useFetch to encapsulate and reuse the logic for loading data and "loading more".
  • This fixes a bug we have in quizzes' resource selection where if you have more than 25 bookmarks, the component will just display "25 bookmarks" and during resource selection you won't be able to load more.
• Fixes a bug we have in quizzes' resource selection where if we had to "load more" resources in the topic tree view, it wouldn't pass the more params correctly and would reload the same resources infinitely.
• Fixes scrollbar styles that appears when you need to scroll in the side panel.

Screencast

Compartir.pantalla.-.2024-12-12.12_05_41.mp4

References

Closes #12790.

Reviewer guidance

How does this new strategy look?

[nucleogenesis]

I'm loving the architecture here - things read really cleanly. I know this is in Draft so I can come back for a more thorough review once it's ready. Great work Alex!

[nucleogenesis]

Not a blocking comment, just want to share a thought I've been having around this kind of pattern that is relatively common in our code.

These objects are structured very much alike to a VueRouter route which makes me wonder if we should consider using VueRouter to handle this behavior instead. Mostly this comes from my sense that this is a job that the VueRouter is purpose-built to handle.

That all said, my meager attempts at doing this myself have run into issues so just something to think about, particularly as we move toward redesigning the SidePanelModal itself.

[AlexVelezLl]

Yees, thats true, I also thought about that, my first thought was that it was gonna be easier to have the business logic to manage the guards or the back page inside the component rather than doing it in the routes array. But I am gonna give it a try and actually see the implications of moving this as route children.

[nucleogenesis]

This is a really clean component overall - I really appreciate the simplicity in how the composable modules are implemented <3

[nucleogenesis]

If the user has to explicitly save their changes then we'll need to handle the "Are you sure?" KModal to confirm losing changes.

[marcellamaki]

Hi Alex, I have added some feedback which is more about discussion than requesting specific changes. I look forward to your thoughts and chatting more! I am still thinking through the provide/inject stuff and will see if I have more coherent ideas to add tomorrow

[marcellamaki]

This isn't a criticism of the code, which seems well organized and nicely written. I'm just wondering what are the pros and cons of including the composable refactor here. What is made easier/possible by doing this? How much harder would it be if we didn't refactor this?

I mean this more about chatting things through a bit more, rather than a simple "yes do this" or "don't do this"

[marcellamaki]

I think a real pro of using composables is that it can clean up files, and make these helper functions reusable. And for things we are doing repeatedly, that makes a lot of sense and is valuable. Overall, I think this work does that well, and it's a pattern that we've been trying to move towards over time, so that's great.

one of the cons from my perspective is that sometimes by making the code very neat, it can (in a sort of unexpected way) be a little bit harder to follow what exactly might be happening and "run the code in your head" when trying to read through and understand (and potentially debug a problem in a specific scenario). This can be extra challenging when it is an abstracted helper function that is not in the context of say, a particular vue file.

if (additionalDataKeys) {
      additionalData.value = Object.entries(additionalDataKeys).reduce((agg, [key, value]) => {
        agg[key] = value === '' ? response : get(response, value);
        return agg;
      }, {});
    }
  };

I don't think there's an easy answer, I think for each case, it's a bit of a balancing act between abstraction, readability, "friendliness", and context.

[AlexVelezLl]

I havent got into documenting this yet as it was still a proposal, but I will need to do this to add context of what this is doing here and why.

What is made easier/possible by doing this? How much harder would it be if we didn't refactor this?

The main benefit of doing this is encapsulation and code reusability. Fetching data is a very common thing we need to do. And in the case we also need to handle the "load more" pattern, we will end up repeteating a lot of code.

For example, for lessons resource selection we need to load data from bookmarks, from channels and from the topic tree (and in the future from search results). So for each of them we will need to write in our composable something like:

const loadingBookmarks = ref(false);
const bookmarks = ref(null);
const bookmarksCount = ref(0);
const bookmarksError = ref(null);
const loadingMoreBookmarks = ref(false) // I will explain better why of this lodingMore.
const loadMoreBookmarksParams = ref(null);

const fetchBookmarks = async () => {
  try {
    loadingBookmarks.value = true;
    const response = await ContentNodeResource.fetchBookmarks({
          params: { limit: 25, available: true },
        }),
    bookmarks.value = response.data;
    bookmarksCount.value = response.count;
    loadMoreBookmarksParams.value = response.more;
  } catch (e) {
    error.value = e;
  }
  loadingBookmarks.value = false
}

const fetchMoreBookmarks = async () => {
  try {
    loadingMoreBookmarks.value = true;
    const response = await ContentNodeResource.fetchBookmarks({
          params: loadMoreBookmarksParams.value,
        }),
    bookmarks.value = response.data;
    bookmarksCount.value = response.count;
    loadingMoreBookmarksParams.value = response.more;
  } catch (e) {
    error.value = e;
  }
  loadingMoreBookmarks.value = false
};

And then we will need to repeat these 25 lines of code to fetch from topic tree, and many of them to fetch the channels, that at the end is a lot of repeated code for a common patern: fetching data, that usually do pretty much the same: starts loading, fetch data, set response, handle errors, finish loading. And at the very end we will end up returning something like this from the composable (just an example of how big it can be, in this particular case we dont need the loading and error states in separated variables).

return {
  loadingBookmarks,
  bookmarks,
  bookmarksCount,
  bookmarksError,
  loadingMoreBookmarks,
  fetchBookmarks,
  fetchMoreBookmarks,
  loadingResources,
  resources,
  topic,
  resourcesError,
  loadingMoreResources,
  fetchResources,
  fetchMoreResources,
  loadingChannels,
  channels,
  channelsError,
  fetchChannels
};

So its not a lot of repeated code, but a lot of variables, thats why encapsulation is another benefit of using this useFetch, so if we use useFetch to load bookmarks, topic tree and channels we will need just this to load all of them:

  const bookmarksFetch = useFetch({
    fetchMethod: () =>
      ContentNodeResource.fetchBookmarks({
        params: { limit: 25, available: true },
      }),
    fetchMoreMethod: more =>
      ContentNodeResource.fetchBookmarks({
        params: more,
      }),
    dataKey: 'results',
    moreKey: 'more',
    additionalDataKeys: {
      count: 'count',
    },
  });

  const channelsFetch = useFetch({
    fetchMethod: () =>
      ChannelResource.fetchCollection({
        getParams: {
          available: true,
        },
      }),
  });

  const treeFetch = useFetch({
    fetchMethod: () =>
      ContentNodeResource.fetchTree({ id: topicId.value, params: { include_coach_content: true } }),
    fetchMoreMethod: more => ContentNodeResource.fetchTree({ id: topicId.value, params: more }),
    dataKey: 'children.results',
    moreKey: 'children.more.params',
    additionalDataKeys: {
      topic: '', // return the whole response as topic
    },
  });

Which is much less repeated code. And if we want to access bookmarks data we need to just access bookmarksFetch.data.

And why prefer these separated loading states instead of having just one general loading? Because as we are working with serveral models to fetch, we can get into race conditions if we need to load several things at the same time.

And another aspect, I have chosen to use these "fetch objects" (treeFetch, channelsFetch, bookmarksFetch) instead of destructuring the results to avoid the big return I showed before. I know this particular object is something we will need to document well as its a new pattern, and its not that obvious that these object contains the loading data objects and methods, but the thing is that for each model there are around 7 related objects, and I think encapsulating them is a good way to manage them without having to write a lot of code. Long retun objects are also hard to read. And also this standarize the variables names of this pattern and make it easier to handle dynamic data. For example allows something like

kolibri/kolibri/plugins/coach/assets/src/views/lessons/LessonSummaryPage/UpdatedResourceSelection.vue

Lines 64 to 86 in 560bedb

	const contentFetch = computed(() => {
	const contentSources = {
	[ResourceContentSource.BOOKMARKS]: bookmarksFetch,
	[ResourceContentSource.TOPIC_TREE]: treeFetch,
	};
	return contentSources[props.source];
	});
	const contentList = computed(() => {
	const { data } = contentFetch.value;
	return data.value;
	});
	const viewMoreButtonState = computed(() => {
	const { moreState } = contentFetch.value;
	return moreState.value;
	});
	function fetchMoreResources() {
	const { fetchMore } = contentFetch.value;
	fetchMore?.();
	}

. That instead of mapping each data object, loading object, fetchMoreMethod, we just map one fetch object and then we can in an abstract way access all these variables that are common to data fetching.

Now in specific why do we need these "moreKey" and "dataKey"? Because the response objects from the api are not standarized. And for channels the response itself is the channels array, for bookmarks the bookmarks array resides in response.results and for resources we need to look at response.children.results, the same happen with the more object. And why have I added this additionalDataKeys, its because in some cases the response can contain additional data like for example the bookmarks that also returns the bookmarks count, and for the resources that also returns the topic data.

I know that these specific lines of code are a little bit complex to read, and apologies for forgetting to add some comments here 😅. What we do here is to map this additionalDataKeys to an additionalData object that contains the data requested in theadditionalDataKeys.

if (additionalDataKeys) {
      additionalData.value = Object.entries(additionalDataKeys).reduce((agg, [key, value]) => {
        agg[key] = value === '' ? response : get(response, value);
        return agg;
      }, {});
    }
  };

Anyways I can rewrite this mapping, as there are ways to make it more easy to understand avoiding the reduce method. And after documenting some of this, it would be easier o follow. Because yes at the end abstractions are a little bit harder to follow as we need to think in terms of the abstracted concept.

[rtibbles]

I think, ultimately, something like this would be useful to integrate into the APIResource layer itself, rather than having to wrap further around that.

The concerns that @marcellamaki describes also apply to a lot of the APIResource layer in general as well - because it becomes hard to reason about what is actually happening when we do a fetch from the backend, because quite a lot happens in the intervening layer - so allowing for the APIResource layer to directly be consumed as a composable in some way might make this feel neater, as you don't have to do the injection into the conmposable - you just get it back from it.

[AlexVelezLl]

Hadnt think about it! I think its a good idea, and more now that we are incrementaly using more composables. Is this something we can explore more in a different issue? Or is expected as a result of this PR?

[rtibbles]

Definitely out of scope for this PR! We can use this as a starting place to explore what this might look like more generally.

[marcellamaki]

I want to think a little bit more about the benefits (and challenges) if using provide/inject. We do use it in other places, so it isn't that we can't, but to Richard's comment in our chat, it's probably worth making sure this is not easily achieved another way. So. I'm going to reflect and come back to this

[AlexVelezLl]

I can write a version without the provide/inject, so we can weight better the implications of each pattern :)

[AlexVelezLl]

I just pushed a commit here in another branch removing the provide/inject: AlexVelezLl@5a94a59. Its not that bad as I first enviosioned, there are just 13 more lines of code, and although this difference will grow while we add more subPages, I'm leaning towards thinking that it is worth removing the provide/inject in favor of props passing. We can discuss more about that.

[marcellamaki]

great - thank you Alex! I'll take a look at that :)

[marcellamaki]

I've only done a quick code read through (more about overall approach of props rather than provide/inject, and not reading each line for comprehension/making sure it works) but I think this might be the way to go for now. I do think that you are right, the diff will grow, but we can figure out if and when it becomes too complex to manage. Perhaps you can stash your provide inject setup away somewhere just in case :D

[marcellamaki]

Part of the reason I'm saying this is in connection to the composable changes that we were discussing above, and weighing the pros/cons of making various changes, and which to make now vs. later. I think you lay out a good reason for using a composable for the fetching logic (although I do want to do another read and make sure I'm really understanding it) and the values of that being modular and re-usable. It's not a huge refactor, and it's aligned with this work.

For me, that seems like the "new" pattern/code to prioritize, over also introducing provide inject here. I think that approach will allow us to have the best balance of benefits of the refactoring which we really want as a core part of this project, without introducing so many changes this goes from refactor to rewrite, which I think is a different thing (and always really tempting for me.... 😸 ). What you're moving toward based on your own reflections as well as this feedback and conversation seems like a good middle ground :)

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0a0.dev0_git.260.gcbf6279b.pex
Windows Installer (EXE)	kolibri-0.18.0a0.dev0+git.260.gcbf6279b-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0a0.dev0+git.260.gcbf6279b-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0a0.dev0+git.260.gcbf6279b.dmg
Android Package (APK)	kolibri-0.18.0a0.dev0+git.260.gcbf6279b-0.1.4-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.18.0a0.dev0+git.260.gcbf6279b.zip
TAR file	kolibri-0.18.0a0.dev0+git.260.gcbf6279b.tar.gz
WHL file	kolibri-0.18.0a0.dev0+git.260.gcbf6279b-py2.py3-none-any.whl

[marcellamaki]

a few comprehension questions as I am digging into the code more deeply now than when it was in the proposal stage!

In the UI the changes look great, just want to be sure the code review is really comprehensive :)

[marcellamaki]

// return the whole response as topic

i'm not sure what this comment means

[AlexVelezLl]

Continuing the response in https://github.com/learningequality/kolibri/pull/12895/files#r1887532711.

If for some reason we would want this whole response object:

{
  results: [ ... ], // lets say limited to 25 bookmarks per fetch
  count: 432.
  more: {...}
}

in just one variable in the additionalDataKeys object. Then we can use the empty string (that we use to get the nested object keys) to say that we dont want anything nested in the response object, but the whole response object for that key.

so if we have

additionalDataKeys = {
  count: 'count'
}

then additionalData.count would be: 432. But if we have

additionalDataKeys = {
  response: '' //empty string key
}

then additionalData.response would be { results: [...], count: 432, more: {...}}

[AlexVelezLl]

nvm Richard just suggested a better alternative to avoid this. So this does not longer matters! 😆.

[marcellamaki]

So here is INDEX the page a user would see when they are in a "selectable" state (i.e. in the topic tree, with checkboxes), the bookmarks is of course that, and the CHANNELS is the initial "landing" page when the side panel opens with the link to the bookmarks as well as the channel cards there?

[AlexVelezLl]

Oh no. I probably need to rename this. INDEX is the first page a user see when they open the select resources side panel. The one that lets them choose to select from bookmarks or select from channels. CHANNELS is the view when we are selection from channels, i.e. when we see the topic tree and the checkboxes. I called it "CHANNELS" because the title of that view is "select from channels".

[marcellamaki]

Well, we don't have to do the version that I think, since we seem to think the opposite thing is what makes the most sense! I'm not necessarily correct. Maybe INDEX could continue to be the landing page (which does make sense) and then CHANNELS could become something like TOPIC_TREE?

[AlexVelezLl]

Makes sense to me. Updated! :)

[marcellamaki]

This comment/example is so helpful, thank you for adding it

[marcellamaki]

I'm not fully understanding the distinction between the example above, where it seems to be passing the moreKey object directly into useFetch()

useFetch({ moreKey: "more" })

and below:

 * const fetchMoreMethod = (more) => ContentNodeResource.fetchBookmarks({
 *  params: more
 * })

where there is a distinct method.

it seems like there's also a fetchMore which is using the fetchMoreMethod, and I can see from the documentation

fetchMore A method to manually trigger fetch more data.

So presumably this would be for something that would be triggered through the UI, i.e. a button click "View more"

this isn't really a question I guess, just trying to wrap my mind around the connections between each of these.

[marcellamaki]

Is this intention here to help the response already be somewhat "pre-processed" if needed, without having to potentially again transform the object, or remap responses to different names (i.e. managing variable name differences between the front and back end, theDifferencesBetween cases_on_front_and_back_end, etc.)?

[AlexVelezLl]

No. The intention is to separate the actual "data" (i.e. if we are fetching bookmarks, have in the data ref the actual array of bookmarks) from the rest of the response (i.e. the response of the bookmarks fetch can contain additional data like the count that tells the total amount of bookmarks, not just the length of the responded array), in part to have a cleaner api, and also to improve the process of "fetching more data".

For example when fetching bookmarks we can get a response like:

{
  results: [ ... ], // lets say limited to 25 bookmarks per fetch
  count: 432.
  more: {...} // this more object will contain the query needed to fetch more data or to fetch the "next page"
}

Then we want data to be the bookmarks array, no the full response, so when we fetch more bookmarks using the fetchMore() method, we already have the actual array in the data ref, and we know how to merge them. Thats why in this case dataKey = "results".

But, apart from just the array of bookmarks, in the UI we also show the total number of bookmarks the user has, which is different from the data.length value, because the response is paginated. So to know the total number of bookmarks we need can use the count key that comes in the same response. Apart from that we can imagine that the endpoint also returns the weight of the bookmarks in a key weight or any other metadata apart from the actual array of bookmarks. As we want to show this "additional data" (additional to the main data which is the bookmarks array) we need to query them and store them in a ref. But the problem is that this data is dynamic, so its difficult to create a count ref, a weight ref, etc... to store all this additional data. For this we have this additionalDataKeys object, that will tell to the useFetch something like "hey apart from the bookmarks array I also want the "count" data that is stored in "response.someNestedObject.count"". So we write a additionalDataKeys key-value pair object like { count: "someNestedObject.count" } to tell that we want the "response.someNestedObject.count" data stored in the "count" key in the "additionalData" object that we return from the useFetch.

So after we fetch bookmarks we can use this count.

const { additionalData, .... } = useFetch(...)

console.log(additionalData.count) // 432.

[rtibbles]

My sense here is that the additionalDataKeys is a premature abstraction at this point - we are only using it twice in this PR, and for seemingly very different purposes, one is to return a specific piece of response data - the count, the other is to return the entirety of the response data.

This suggests to me that trying to abstract away everything behind options configuration might be a wrong turn, and we should limit that to the truly repeated functionality - loading state, loading state for more items for pagination, and maybe fetching more.

[AlexVelezLl]

The best alternative I can think of is to return the whole response object apart from the data ref, how does that sounds?

[rtibbles]

See my suggestion for fetchTree specifically - I think that does away with the need for a lot of the abstraction here, count we can just always return and just have it be null in the case when the pagination is cursor based, and does not provide a count.

[AlexVelezLl]

Updated :). Thank you @rtibbles 👐

[marcellamaki]

limit: 25

I think I've answered my own question and that you're talking about pagination

[rtibbles]

From an API consumption perspective, is it confusing that the bookmarks fetching uses the limit parameter, whereas all the other contentnode APIs use max_results?

Under the hood, this is a result of the Bookmarks contentnode API using Limit/Offset pagination, whereas all the others use a more cursor style pagination, I am just not sure this is helpful to the consumer :)

[marcellamaki]

I think what's confusing is, at least to me, more is conceptually consistent but literally (in the code) not always consistent. So yes, some of it is about limit vs. max_results but also.... I don't have a great mental model of what can be in that object. When I see something like data.more or this.more, it's really hard for me to track if that's results (the response of say, the second page) or if that's the more params.

this isn't a criticism, @AlexVelezLl of your code -- I think you are in line with the existing usage. It's just something I have always sort of struggled with in terms of readability wherever we use this general pagination pattern. I just have to read and re-read things so many times to follow along.

[rtibbles]

From the API perspective more is always either null or an object containing the exact GET parameters to retrieve the next page from the API - if we're using it to mean other things in the frontend, that's probably not helpful.

[AlexVelezLl]

@marcellamaki Do you think that renaming this more identifier to something like moreParams would help to make the intention of this object clearer?

[marcellamaki]

yes, I think that would help - not required, but maybe a nice to have

[AlexVelezLl]

super, just updated it to be called moreParams instead :)

[rtibbles]

I am not sure this state gives us anything over just exporting more and loadingMore as simple booleans.

[AlexVelezLl]

Yes, we can export both. It was just to pass it directly to the vue component using the states of the ViewMoreButtonStates, but I dont have a strong preference. And perhaps returning both booleans can be a little more absctract.

[AlexVelezLl]

Updated!

[rtibbles]

children.more just needs to be passed directly to the fetchTree method of ContentNodeResource.

[AlexVelezLl]

oh, yes 😅. I was imitating the useFetchTree and didnt notice this

[AlexVelezLl]

Updated!

[rtibbles]

I think we could simplify the abstractions in useFetch by dealing with the special case for tree fetching in here instead.

We can create a separate topic ref that we assign the whole result of ContentNodeResource.fetchTree to, and then return:

ContentNodeResource.fetchTree({ id: topicId.value, params: { include_coach_content: true } }).then(t => {
   topic.value = t;
   return topic.children;
});

This allows us to avoid any special casing, and the returned data is still a more parameter, so we don't need to customize the moreKey either, and that can be static.

[AlexVelezLl]

Gonna try that!

[AlexVelezLl]

It now looks so much better :)

  const fetchTree = async (params = {}) => {
    topic.value = await ContentNodeResource.fetchTree(params);
    return topic.value.children;
  };

  const treeFetch = useFetch({
    fetchMethod: () => fetchTree({ id: topicId.value, params: { include_coach_content: true } }),
    fetchMoreMethod: more => fetchTree(more),
    dataKey: 'results',
    moreKey: 'more',
  });

[rtibbles]

Do we need dataKey and moreKey to be configurable now? I have deliberately made all the API endpoints use consistent return values for these, so I think it shouldn't need to be!

[AlexVelezLl]

Are them? Wow, Just for the answer of the channels endpoint I thought they were not. Because there the data is the whole response object. But I guess in that case different responses are because that response is not paginated right? So we can assume that if we dont send a "fetchMoreMethod", then the data is just the whole response object, like the channels response. And if we do send a "fetchMoreMethod", then the data will always come in "response.results", the more object in "response.more", and the count in "response.count", right?

[AlexVelezLl]

That consistency in the api responses will make this composable so much less complex :D. At the beginning I thought they were not because of the TopicTree response, but after that nice solution of how we can treat it, everything will become so much easier. Thanks!

[rtibbles]

Just 2-3 small things I noticed.

[rtibbles]

One minor nit here - this feels like this should just be true rather than deferring to the value of loadingMore.value.

[AlexVelezLl]

solved!

[rtibbles]

We should add a guard here against loading twice, I think? If loading.value is already true, we shouldn't try to load again?

[AlexVelezLl]

At the end, with @rtibbles we decided to give priority to the latests fetch and ignore previous fetches if concurrent fetches has happened.

[rtibbles]

Similarly here, if we are already trying to loadMore, then we shouldn't load more again until that's resolved?

[AlexVelezLl]

done!

[marcellamaki]

Thank you, @AlexVelezLl !! 👏