Skip to content
This repository has been archived by the owner on Jul 25, 2024. It is now read-only.

Nullable fields fixed, donors special case fixed #194

Merged
merged 6 commits into from
Sep 29, 2021
Merged

Nullable fields fixed, donors special case fixed #194

merged 6 commits into from
Sep 29, 2021

Conversation

engelke
Copy link
Contributor

@engelke engelke commented Sep 27, 2021

  1. The API server allowed some fields to be Null (None in Python) but the openapi.yaml spec did not note that, so the client libraries threw exceptions. The openapi.yaml spec has been corrected.

  2. The donors resource is a special case: the non-id attribute "email" should be unique in the collection. The "insert" operation for donors now checks to see if there is a donor with a matching email and, if so, updates the existing donor instead of creating a new one.

This helps API clients that want to create a donation, and need to know the donor ID for that. They can just POST a donor and get back the resource representation.

@engelke engelke requested a review from a team as a code owner September 27, 2021 21:22
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 27, 2021
Copy link
Collaborator

@grayside grayside left a comment

Choose a reason for hiding this comment

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

Spec changes look reasonable to me, but I'm a bit unsure about donor POST representing create or update.

I get the rationale that a 4xx error followed by a PATCH is an extra round trip, but this feels like a snowflake.

Should we document a design philosophy that POST where we specify a unique property that already exists that we treat it like a PUT? If so this seems like it should have a follow-up to cover approvers too.

@ace-n
Copy link
Contributor

ace-n commented Sep 27, 2021

@engelke request: would you mind splitting this up into two commits - one with your changes, and one with the generated ones?

(At least for me, that would make this easier to navigate.)

@engelke
Copy link
Contributor Author

engelke commented Sep 28, 2021

For reviewers:

  • All client-libs files except for python/emblem_client/init.py are generated from openapi.yaml, so review that .yaml, not the generated files
  • contest-api/cleanupdb is just a utility to use while testing, and in a perfect world would not be needed
  • data/cloud_firestore.py now prefixes collection names with a random uuid when running tests and corrects one response from 201 to 200
  • .test files now expect an oauth2 or firebase token, not a firebase session
  • main_approver_test.py now cleans up in the fixture
  • main_nonapprover_test.py is entirely new, and has lots of complicated authorization checks
  • both tests have fixes related mostly to donations, which are full of special cases
  • content-api/resources/auth.py and content-api/resources/methods.py fix a lot of issues the new tests unearthed

This also incorporates the content-api updates from #109, which I'm going to close unmerged. That PR also has some website fixes that I think Ace has handled in a separate PR.

So this isn't as horrible as it looks.

@engelke engelke mentioned this pull request Sep 28, 2021
Committed by mistake
Comment on lines +146 to +150
# Tear down test data
db.delete("causes", TEST_CAUSE["id"], resource_fields["causes"], None, host_url=None)
for email_address in [EMAIL, "nobody@example.com"]:
db.delete("campaigns", TEST_CAMPAIGN[email_address]["id"], resource_fields["campaigns"], None, host_url=None)
db.delete("donors", TEST_DONOR[email_address]["id"], resource_fields["donors"], None, host_url=None)

Choose a reason for hiding this comment

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

Suggested change
# Tear down test data
db.delete("causes", TEST_CAUSE["id"], resource_fields["causes"], None, host_url=None)
for email_address in [EMAIL, "nobody@example.com"]:
db.delete("campaigns", TEST_CAMPAIGN[email_address]["id"], resource_fields["campaigns"], None, host_url=None)
db.delete("donors", TEST_DONOR[email_address]["id"], resource_fields["donors"], None, host_url=None)
# Tear down test data
db.delete(
"causes", TEST_CAUSE["id"], resource_fields["causes"], None, host_url=None
)
db.delete(
"campaigns",
TEST_CAMPAIGN[email_address]["id"],
resource_fields["campaigns"],
None,
host_url=None,
)
db.delete(
"donors",
TEST_DONOR[email_address]["id"],
resource_fields["donors"],
None,
host_url=None,
)
db.delete(
"donations",
TEST_DONATION[email_address]["id"],
resource_fields["causes"],
None,
host_url=None,
)

r = client.delete("/donations/{}".format(donation["id"]), headers=headers)
assert r.status_code == 204
r = client.delete("/donors/{}".format(donor["id"]), headers=headers)
assert r.status_code == 204

Choose a reason for hiding this comment

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

Suggested change
assert r.status_code == 204
assert r.status_code == 204

@engelke
Copy link
Contributor Author

engelke commented Sep 28, 2021

@grayside There are some clumsy behaviors necessitated by missing functionality in the generated client libraries. However, I will add them to the emblem_api in a future PR, at which time we can eliminate the special POST /donors behavior and provide a way to see "synthetic" fields like the total donations in a campaign.

@engelke engelke merged commit 51a3f43 into main Sep 29, 2021
@engelke engelke deleted the nullables branch September 29, 2021 21:41
Comment on lines +146 to +150
# Tear down test data
db.delete("causes", TEST_CAUSE["id"], resource_fields["causes"], None, host_url=None)
for email_address in [EMAIL, "nobody@example.com"]:
db.delete("campaigns", TEST_CAMPAIGN[email_address]["id"], resource_fields["campaigns"], None, host_url=None)
db.delete("donors", TEST_DONOR[email_address]["id"], resource_fields["donors"], None, host_url=None)

Choose a reason for hiding this comment

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

Suggested change
# Tear down test data
db.delete("causes", TEST_CAUSE["id"], resource_fields["causes"], None, host_url=None)
for email_address in [EMAIL, "nobody@example.com"]:
db.delete("campaigns", TEST_CAMPAIGN[email_address]["id"], resource_fields["campaigns"], None, host_url=None)
db.delete("donors", TEST_DONOR[email_address]["id"], resource_fields["donors"], None, host_url=None)
# Tear down test data
db.delete(
"causes", TEST_CAUSE["id"], resource_fields["causes"], None, host_url=None
)
db.delete(
"campaigns",
TEST_CAMPAIGN[email_address]["id"],
resource_fields["campaigns"],
None,
host_url=None,
)
db.delete(
"donors",
TEST_DONOR[email_address]["id"],
resource_fields["donors"],
None,
host_url=None,
)
db.delete(
"donations",
TEST_DONATION[email_address]["id"],
resource_fields["causes"],
None,
host_url=None,
)

r = client.delete("/donations/{}".format(donation["id"]), headers=headers)
assert r.status_code == 204
r = client.delete("/donors/{}".format(donor["id"]), headers=headers)
assert r.status_code == 204

Choose a reason for hiding this comment

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

Suggested change
assert r.status_code == 204
assert r.status_code == 204

@grayside grayside added this to the v0.5.0 milestone Nov 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants