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

Breadcrumbs not updated when users updating area name #213

Closed
vnugent opened this issue Jan 1, 2023 · 8 comments
Closed

Breadcrumbs not updated when users updating area name #213

vnugent opened this issue Jan 1, 2023 · 8 comments
Assignees
Labels
complexity - Medium help wanted Extra attention is needed
Milestone

Comments

@vnugent
Copy link
Contributor

vnugent commented Jan 1, 2023

When an area is created, we also generate its bread crumbs data in pathTokens field. However, when we update an area's name, we don't update pathTokens.

openbeta-breadcrumbs-bug

To do:

  • Case 1: The area has no child areas. Simply update pathTokens when we update area's name.
  • Case2: The area has child areas. We need to update all of its children's pathTokens
@vnugent vnugent added help wanted Extra attention is needed complexity - Medium labels Jan 1, 2023
@vnugent vnugent added this to the v0.7 milestone Jan 28, 2023
@CocoisBuggy
Copy link
Contributor

I can fix this but I just want to make absolutely sure we don't want to resolve this at query time rather than at mod time?

We chatted about reads being far more frequent than writes / modifications but I just want to make sure that the pathTokens system is still the way we want to go?

@vnugent
Copy link
Contributor Author

vnugent commented Mar 19, 2023

I can fix this but I just want to make absolutely sure we don't want to resolve this at query time rather than at mod time?

We chatted about reads being far more frequent than writes / modifications but I just want to make sure that the pathTokens system is still the way we want to go?

Correct. We need to update pathToken, either iteratively or recursively, when doing updateArea(),

@saferthanhouses
Copy link

@vnugent Happy to look into this if it's available.

@zichongkao
Copy link
Contributor

@saferthanhouses @CocoisBuggy I know it's been a while since we've updated this issue. Are either of you still open to taking it on?

@andrew-jp
Copy link
Contributor

andrew-jp commented Jun 9, 2023

I'm new here/have no idea what I'm doing, but I've been working on this all day. What do you guys think of the general idea?

  updateArea()...
  ...
      const area = await this.areaModel.findOne(filter).session(session)
      if (area == null) {
        throw new Error('Area update error.  Reason: Area not found.')
      }

      const { areaName, description, shortCode, isDestination, isLeaf, isBoulder, lat, lng, experimentalAuthor } = document

  ...
      if (areaName != null) {
        const sanitizedName = sanitizeStrict(areaName)
        area.set({ area_name: sanitizedName })
        area.set({ pathTokens: area.pathTokens.slice(0, -1).push(sanitizedName) })

        if (area.children.length > 0) {
          await this.updatePathTokens(area.children, sanitizedName)
        }
      }

    ...
  async updatePathTokens (areaChildren: Types.ObjectId[], areaName: string, index: number = 2): Promise<void> {
    const session = await this.areaModel.startSession()

    for (const childId of areaChildren) {
      const filter = { _id: childId }

      const area = await this.areaModel.findOne(filter).session(session)
      if (area != null) {
        const newPath = area.pathTokens
        newPath[newPath.length - index] = areaName
        area.set({ pathTokens: newPath })

        if (area.children.length > 0) {
          await this.updatePathTokens(area.children, areaName, index + 1)
        }
        // save the changes
        await area.save()
      }
    }
  }

I really have no idea what I'm doing here, but I'm trying to figure it out. I would test it, but I believe I need API keys to edit anything even in a local env.

@vnugent
Copy link
Contributor Author

vnugent commented Jun 9, 2023

@andrew-jp thanks for taking on this important issue. I think you're in the right direction.

Can you send me an email: viet at openbeta dot io ? I'll reply with API keys.

@andrew-jp
Copy link
Contributor

andrew-jp commented Jun 12, 2023

My solution works! Just had to use a more JS/TS-like array copy. It looks good in testing so far, although recursion with queries/async operations involved is very expensive/slow. I'll see if I can improve the performance a bit.

Additionally, I'm trying to figure out if I need to use sessions in the "save" operation of the recursive function. Does the session protect/validate the Mongo transaction? I read some docs, but their purpose isn't totally clear to me. Any advice here would be helpful.

Central Wasatch - 20 (large) areas/3200 climbs - ~7.5s
Little Cottonwood Canyon - 59 areas/1800 climbs - ~4s
Both timed in real time - not the actual backend op.
Screen Shot 2023-06-12 at 12 39 07 PM
Screen Shot 2023-06-12 at 12 39 22 PM

andrew-jp added a commit to andrew-jp/openbeta-graphql that referenced this issue Jun 12, 2023
@vnugent
Copy link
Contributor Author

vnugent commented Jun 12, 2023

@andrew-jp great to hear!

In general it's not going to be perfomant when making individual db calls for each child.

In async updatePathTokens() I think you might be able to eliminate the for loop and multiple find() calls by either:

  1. Calling mongoose populate() function to turn the array of area IDs into array of area objects.
  2. Using the $in operator to get all area objects in 1 call

Similar code
https://github.com/OpenBeta/openbeta-graphql/blob/develop/src/db/utils/jobs/TreeUpdater.ts#L47

andrew-jp added a commit to andrew-jp/openbeta-graphql that referenced this issue Jun 13, 2023
andrew-jp added a commit to andrew-jp/openbeta-graphql that referenced this issue Jun 13, 2023
andrew-jp added a commit to andrew-jp/openbeta-graphql that referenced this issue Jun 14, 2023
vnugent pushed a commit that referenced this issue Jul 4, 2023
* refactor: use mongoose 'populate()' to hydrate children array
* added basic unit test for (#213) - verify pathToken change through tree
* simplified array indexing since the value is constant, passes tests
@vnugent vnugent closed this as completed Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity - Medium help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants