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

changes of pg-promise #816

Merged
merged 1 commit into from Dec 26, 2019
Merged

changes of pg-promise #816

merged 1 commit into from Dec 26, 2019

Conversation

@paninee
Copy link
Contributor

paninee commented Dec 26, 2019

No description provided.

@paninee paninee merged commit 99cfb52 into master Dec 26, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Copy link
Contributor

vitaly-t left a comment

Totally invalid code change, here, and elsewhere....

@paninee

This comment has been minimized.

Copy link
Contributor Author

paninee commented Dec 29, 2019

@vitaly-t invalid how?

]);
t.none(INSERT_AUTHOR, author),
t.none(INSERT_LOCALIZED_TEXT, updatedText),
t.none(UPDATE_CASE, updatedCase)
});

This comment has been minimized.

Copy link
@ascott

ascott Jan 3, 2020

Contributor

@paninee this doesn't look correct to me. looking at the original PR these lines should be:
https://github.com/participedia/api/pull/773/files#diff-07780e70b8cc879c37de679a75e0e6bbR235

await db.tx("update-organization", async t => {
  await t.none(INSERT_AUTHOR, author);
  await t.none(INSERT_LOCALIZED_TEXT, updatedText);
  await t.none(UPDATE_ORGANIZATION, updatedOrganization);
});

@paninee i'm curious, why didn't you merge the original PR's?
#775
#774
#773

This comment has been minimized.

Copy link
@vitaly-t

vitaly-t Jan 3, 2020

Contributor

@pan this doesn't look correct to me. looking at the original PR these lines should be:
https://github.com/participedia/api/pull/773/files#diff-07780e70b8cc879c37de679a75e0e6bbR235

await db.tx("update-organization", async t => {
  await t.none(INSERT_AUTHOR, author);
  await t.none(INSERT_LOCALIZED_TEXT, updatedText);
  await t.none(UPDATE_ORGANIZATION, updatedOrganization);
});

@pan i'm curious, why didn't you merge the original PR's?
#775
#774
#773

Now that looks right. The code I mentioned in the beginning looks very odd.

This comment has been minimized.

Copy link
@paninee

paninee Jan 4, 2020

Author Contributor

@ascott I didn't just merge the 3 original PRs because when I checked they were already merged before I started this task #809. So I assume I need to make changes to how we are using db.tx and t.batch from the pg-promise lib.

This comment has been minimized.

Copy link
@ascott

ascott Jan 4, 2020

Contributor

@paninee hmm, not sure how/why they appeared to be already merged since the PRs are still open and not merged. I would suggest pulling these 3 PRs (#775, #774, #773) into a new branch, resolving conflicts and make sure the tests are passing, and then opening a new PR. Please tag me and I will take a look before merging into master. Thanks!

This comment has been minimized.

Copy link
@paninee

paninee Jan 4, 2020

Author Contributor

Will do. Thanks @ascott

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.