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: use upsertpins #938

Merged
merged 5 commits into from
Feb 22, 2022
Merged

fix: use upsertpins #938

merged 5 commits into from
Feb 22, 2022

Conversation

GaryHomewood
Copy link
Contributor

@GaryHomewood GaryHomewood commented Feb 1, 2022

We were looping a list of pins and using upsertPin to change the status to 'Pinned'. Instead we should use upsertPins to batch update all of them in one request.

@GaryHomewood GaryHomewood requested a review from flea89 February 1, 2022 15:32
@GaryHomewood GaryHomewood self-assigned this Feb 1, 2022
@GaryHomewood GaryHomewood linked an issue Feb 2, 2022 that may be closed by this pull request
Copy link
Contributor

@flea89 flea89 left a comment

Choose a reason for hiding this comment

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

Please hold merging, I might have approved too soon. 👀

packages/api/src/utils/pin.js Outdated Show resolved Hide resolved
@GaryHomewood GaryHomewood requested a review from flea89 February 11, 2022 15:12
@flea89 flea89 assigned flea89 and unassigned GaryHomewood Feb 14, 2022
packages/db/index.js Outdated Show resolved Hide resolved
packages/db/index.js Show resolved Hide resolved
@GaryHomewood GaryHomewood requested a review from flea89 February 16, 2022 14:19
Copy link
Contributor

@flea89 flea89 left a comment

Choose a reason for hiding this comment

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

LGTM,
thanks!

@flea89 flea89 removed their assignment Feb 21, 2022
@flea89 flea89 requested a review from vasco-santos February 21, 2022 12:05
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

I know the motivation to use upsertPins when I read the code. But it is important to give the context to reviewers and community on the PR description. Can you please add it?

}
})
await db.upsertPins(pins)
return pins
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for changing the return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not specifically, so I have reverted it.

packages/cron/src/jobs/pins.js Outdated Show resolved Hide resolved
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Awesome LGTM!

@GaryHomewood GaryHomewood merged commit d593a19 into main Feb 22, 2022
@GaryHomewood GaryHomewood deleted the fix/862-batch-pins-upsert branch February 22, 2022 17:35
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.

Improvement - Upsert list of pins instead of looping
3 participants