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

[Fix]: Migrate existing tag nav to new navigation tree structure #4882

Merged

Conversation

dancastellon
Copy link
Contributor

Resolves #4881
Impact: minor
Type: bugfix

Issue

With the merge of #4683, a new API for defining and loading navigation trees was introduced. As part of migration 50, a default navigation tree was created with a single "Home" item. The migration should have also converted the existing tag nav items to the new tree.

Solution

This PR updates migration 50 to migrate the existing tag nav to the new tree structure.

Breaking changes

None

Testing

  1. Login to Reaction and update the tag nav to look like the demo storefront: https://storefront.demo.reactioncommerce.com/
  2. In the database, set Migrations.version back to 49 and restart the app
  3. Run this query in Graphiql and confirm the tree structure matches what you see in the Reaction app (Compare the content -> value field)
{
  primaryShop {
    defaultNavigationTree(language: "en"){
      _id
      shopId
      name
      items {
        navigationItem {
          data {
            ...NavigationItemFields
          }
        }
        items {
          navigationItem {
            data {
              ...NavigationItemFields
            }
          }
          items {
            navigationItem {
              data {
                ...NavigationItemFields
              }
            }
          }
        }
      }
    }
  }
}

fragment NavigationItemFields on NavigationItemData {
  content {
    language
    value
  }
  classNames
  url
  isUrlRelative
  shouldOpenInNewWindow
}

@dancastellon dancastellon changed the title [Fix] Migrated existing tag nav to new navigation tree structure [Fix]: Migrate existing tag nav to new navigation tree structure Dec 17, 2018
@dancastellon
Copy link
Contributor Author

@mikemurray and @aldeed Can you take a look at this when you have a chance?

aldeed
aldeed previously requested changes Dec 18, 2018
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dancastellon Can you move the two util functions to /imports/plugins/core/versions/server/util? We don't want migrations to reference any code outside of the versions plugin because otherwise someone may use those utils for something else, change them to match future development, and in the process break the way the original migration is supposed to work.

@mikemurray
Copy link
Member

@dancastellon I noticed that to 2nd level tags aren't sorted in the correct order. Do we need to preserve that order or was it an acceptable loss to sort them by createdAt instead?

@aldeed
Copy link
Contributor

aldeed commented Dec 18, 2018

I vote for preserving the current order, which is the order in the subtag arrays. You could potentially follow the pattern used in the starterkit, which gets all tags first and then passes to the recursive function.

@mikemurray
Copy link
Member

@aldeed should this be going into develop or the release-2.0.0-rc.8 branch?

…eserve order of subtags (order defined on parent)
@dancastellon
Copy link
Contributor Author

@mikemurray and @aldeed - I've made 2 changes:

  • Moved the 2 util functions to the versions plugin
  • Updated the order of sub tags to match order defined on parent (parentTag.relatedTagIds)

To retest you should just be able to:

  • Delete NavigationTrees and NavigationItems records
  • Set Migrations.version back to 49
  • Restart app

@mikemurray
Copy link
Member

@dancastellon @aldeed Looks good to me.

Just wanted to confirm which branch this PR is supposed to go into. release-2.0.0-rc.8 or into develop since it seems like this is one of the last PRs to target release-2.0.0-rc.8

@spencern
Copy link
Contributor

@mikemurray Good question on the base branch for this PR. Based on the fact that #4683 was merged into RC8, I believe this should target RC8 as well.

@mikemurray
Copy link
Member

@spencern then into RC8 it goes

Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mikemurray mikemurray dismissed aldeed’s stale review December 19, 2018 02:05

Files related to the migration have been moved to the versions plugin

@mikemurray mikemurray merged commit b5dfeed into release-2.0.0-rc.8 Dec 19, 2018
@mikemurray mikemurray deleted the fix-4881-dancastellon-navigation-migration branch December 19, 2018 02:06
@spencern spencern mentioned this pull request Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants