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

Contact PUT #112

Merged
merged 9 commits into from
Mar 24, 2021
Merged

Contact PUT #112

merged 9 commits into from
Mar 24, 2021

Conversation

imbstack
Copy link
Contributor

@imbstack imbstack commented Mar 19, 2021

Proposed changes

Should approximately finish #55 (although there are a few discrepancies to clear up now that I look) Maybe worth doing in a followup though

Types of changes

What types of changes does your code introduce?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the guides
  • I have followed the Mozilla Lean Data Policies
  • Lint and tests pass both locally and on the cicd with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Use this space for additional comments, complications, learnings, alternatives,...

@imbstack imbstack marked this pull request as ready for review March 22, 2021 21:15
@imbstack imbstack requested a review from a team as a code owner March 22, 2021 21:15
@imbstack imbstack requested review from jwhitlock and cvalaas and removed request for a team March 22, 2021 21:15
Copy link
Contributor

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

Thanks @imbstack! I tried this in the swagger API. It is challenging due to issue #105, but my basic tests worked (add a minimal contact, update that contact).

The two big issues I see are the PUT path (/ctms vs /ctms/{email_id}), and the failing tests.

I envisioned this endpoint's strategy as "omitting a parameter means leave it alone", where instead it has the strategy of "make it look like what I sent you". That means that you always have to GET the existing object to update, and last writer wins. I think that is less useful for the way Basket uses it, but we can make it work.

ctms/crud.py Show resolved Hide resolved
ctms/models.py Show resolved Hide resolved
tests/unit/test_api.py Outdated Show resolved Hide resolved
tests/unit/test_api.py Outdated Show resolved Hide resolved
tests/unit/test_api.py Outdated Show resolved Hide resolved
ctms/app.py Outdated Show resolved Hide resolved
@imbstack imbstack requested a review from jwhitlock March 23, 2021 20:19
Copy link
Contributor

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

Thanks @imbstack!

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.

3 participants