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

AH-436: update library #39

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

AH-436: update library #39

wants to merge 12 commits into from

Conversation

jeanmurillo95
Copy link

@jeanmurillo95 jeanmurillo95 commented Aug 18, 2022

Surf-shop Change Request

Change Comments

I updated the mongodb library, I also changed some methods and test cases.

Solution description

I updated the mongodb package, I reduced the complexity in some functions, repair some test cases and lint errors.

Linting & unit tests ev

Screen Shot 2022-08-30 at 12 45 59 PM

idence

Checklist

PR is ready for code review and approval when each box is checked.

All steps are required, when applicable. Strikeout formatting indicates a step is not applicable to this change set.

  • Ticket referenced in PR title (ex. AH-XXX: Short summary of change)
  • Not-applicable items in this checklist struck out

Copy link

@candradeg candradeg left a comment

Choose a reason for hiding this comment

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

lgtm

@jeanmurillo95 jeanmurillo95 changed the title fix: update library AH-436: update library Aug 30, 2022
@jeanmurillo95 jeanmurillo95 marked this pull request as ready for review August 30, 2022 18:52
@jeanmurillo95 jeanmurillo95 added the dependencies Pull requests that update a dependency file label Aug 30, 2022
Copy link

@Fernavarro21 Fernavarro21 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

@daniel-hume daniel-hume left a comment

Choose a reason for hiding this comment

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

looks good

@youngcm2 youngcm2 requested review from a team and DanStory and removed request for a team August 31, 2022 22:15
yarn.lock Outdated Show resolved Hide resolved
tests/Repository.spec.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/Repository.ts Outdated Show resolved Hide resolved
src/Repository.ts Outdated Show resolved Hide resolved
src/Repository.ts Outdated Show resolved Hide resolved
src/Repository.ts Outdated Show resolved Hide resolved
src/Repository.ts Outdated Show resolved Hide resolved
tests/Repository.spec.ts Outdated Show resolved Hide resolved
src/Repository.ts Outdated Show resolved Hide resolved
src/Repository.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@DanStory DanStory left a comment

Choose a reason for hiding this comment

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

There is unaddressed comments, make sure to unhide all comments on the Conversation tab

image

@@ -144,7 +145,7 @@ export class MongoRepository<DOC, DTO = DOC> {
const eventResult = await this.invokeEvents(PRE_KEY, [RepoOperation.save, RepoOperation.create], document);
const res = await collection.insertOne(eventResult);

let newDocument = this.toggleId(res.ops[0], false);
let newDocument = this.toggleId({ id: res.insertedId }, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be the whole document, not just the id.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is unresolved

@MatthewEppelsheimer
Copy link

@jeanmurillo95 there appear to still be several unresolved comments from the last review.

@youngcm2
Copy link
Member

@MatthewEppelsheimer @DanStory Can you please look at this PR?

}
let ourCollection;
try {
ourCollection = await db.collection(this.options.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing { strict: true } so it doesn't create the collection out right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants