Enhanced navigation reducer in Volto

[Hrittik20]

Fixes: #5772

[netlify]

✅ Deploy Preview for plone-components ready!

Name	Link
🔨 Latest commit	19e49af
🔍 Latest deploy log	https://app.netlify.com/sites/plone-components/deploys/660e1d565d7f6600072cbecb
😎 Deploy Preview	https://deploy-preview-5817--plone-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[netlify]

✅ Deploy Preview for volto ready!

Name	Link
🔨 Latest commit	19e49af
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/660e1d569c16210008c20b47
😎 Deploy Preview	https://deploy-preview-5817--volto.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[ichim-david]

@Hrittik20 when you add a pull request you need to check the CI tests and find out what fails.

You can see that the unit tests fail for plone/volto.
this means that you need to go inside your branch to packages/volto and run:
pnpm test

jest testing will run and eventually you will get failures.
hit f key and only the failed tests will run.
From there you can make changes to the code or tests and when saving the failed tests will re-run.
This way you will know if what you changed fixed the tests or not.

[Hrittik20]

@ichim-david Yes, I'm working on it. I will soon make an updated PR

[ichim-david]

@ichim-david Yes, I'm working on it. I will soon make an updated PR

@Hrittik20 I think the navigation test can avoid being update as long as the id is filtered since we
have the url field.
@sneridagh what do you think, are we ok with having the id of the item added or
should we remove it?

[Hrittik20]

@ichim-david How about If I update navigation.js by adding a check within the reducer logic that compares the incoming navigation items with the existing ones based on their URLs?

function isSameItem(item1, item2) {
  return item1.url === item2.url;
}
export default function navigation(state = initialState, action = {}) {
  let hasExpander;
  switch (action.type) {
    case `${GET_NAVIGATION}_PENDING`:
      return {
        ...state,
        error: null,
        loaded: false,
        loading: true,
      };
    case `${GET_CONTENT}_SUCCESS`:
      hasExpander = hasApiExpander(
        'navigation',
        getBaseUrl(flattenToAppURL(action.result['@id'])),
      );
      if (hasExpander && !action.subrequest) {
        const newItems = getRecursiveItems(
          action.result['@components'].navigation.items,
        );
        const hasSameItem = newItems.some(incomingItem =>
          state.items.some(existingItem => isSameItem(incomingItem, existingItem))
        );
        if (!hasSameItem) {
          return {
            ...state,
            error: null,
            items: newItems,
            loaded: true,
            loading: false,
          };
        }
      }
      return state;

[ichim-david]

@ichim-david How about If I update navigation.js by adding a check within the reducer logic that compares the incoming navigation items with the existing ones based on their URLs?

function isSameItem(item1, item2) {
  return item1.url === item2.url;
}
export default function navigation(state = initialState, action = {}) {
  let hasExpander;
  switch (action.type) {
    case `${GET_NAVIGATION}_PENDING`:
      return {
        ...state,
        error: null,
        loaded: false,
        loading: true,
      };
    case `${GET_CONTENT}_SUCCESS`:
      hasExpander = hasApiExpander(
        'navigation',
        getBaseUrl(flattenToAppURL(action.result['@id'])),
      );
      if (hasExpander && !action.subrequest) {
        const newItems = getRecursiveItems(
          action.result['@components'].navigation.items,
        );
        const hasSameItem = newItems.some(incomingItem =>
          state.items.some(existingItem => isSameItem(incomingItem, existingItem))
        );
        if (!hasSameItem) {
          return {
            ...state,
            error: null,
            items: newItems,
            loaded: true,
            loading: false,
          };
        }
      }
      return state;

@Hrittik20 I want to hear what @davisagli or @sneridagh has to say about this change, we might be ok to have also the id present without having todo any filtering, or we can simply filter it when destructuring it here https://github.com/plone/volto/pull/5817/files#diff-c1e66aca1aa305458203941b7e7d598522bbecc56ead1841087e2393b93a57b6R37

[sneridagh]

I'd like @tiberiuichim and @davisagli to take a final look as well.

[davisagli]

a minor suggestion, but this lgtm