-
Notifications
You must be signed in to change notification settings - Fork 119
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
feat: log terms of service acceptance #2028
Conversation
a9fd64f
to
2cd3a86
Compare
db97836
to
7d4e041
Compare
6c449f9
to
60e2e75
Compare
7300c9d
to
2e6bc7f
Compare
2e6bc7f
to
d5f42c8
Compare
28634e9
to
0356086
Compare
… payment method' for backend and make website payment page make use of the two different use-cases for calling PUT /user/payment
As I added tests for the different ways of invoking the http api and what should require agreements (vs not), I had to change even more than I realized in backend code (not your stuff, more like my old stuff). Basically because before the two places the frontend did PUT /user/payment were kinda the same. But now they're kinda different because when the AddPaymentForm just wants to update your payment method, that should require an agreement (ie because we don't even show that agreement to the end user in that part of the UI). The other thing that's major is I got rid of the uniquness requirement on the agreement table. Now we'll record the timestamp of each time the user agrees to the thing. It might be more than we need, but it won't be a ton of data that I think should be problematic. The main reason I did this is I think the code that was there before would have had an uncaught error the second time savePaymentSettings was called with same agreement due to the uniqueness constraint in the db, and this was a quick way of avoiding that without having to do more clever queries. |
@e-schneid any concerns about the updates I made? |
async createUserAgreement (userId, agreement) { | ||
const { error } = await this._client | ||
.from('agreement') | ||
.insert({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gobengo As an insert, won't error
be populated if the same user adds the same agreement? Then this will throw and it doesn't seem like it's being caught. It's possible this intended (also possible I'm misreading), but just wanted to call it out just in case.
Everything else looks great! I love the solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of the uniqueness constraint in tables and in the migration. So... I think it won't error in that case eh?
here is the relevant test: 5c84b5b
Or am I misunderstanding?
@gobengo All looks great to me. You considered a lot of things I hadn't. 👍 Just left the one question above regarding insert. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go team 🎉
🤖 I have created a release *beep* *boop* --- ## [7.10.0](api-v7.9.1...api-v7.10.0) (2022-10-23) ### Features * log terms of service acceptance ([#2028](#2028)) ([47c3540](47c3540)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [2.31.0](website-v2.30.0...website-v2.31.0) (2022-10-23) ### Features * AddPaymentMethodForm shows a more end-user-friendly error message when it encounters an error ([#2058](#2058)) ([376e05b](376e05b)) * log terms of service acceptance ([#2028](#2028)) ([47c3540](47c3540)) ### Bug Fixes * clarity on web3.storage PSA ([#2062](#2062)) ([62dadb1](62dadb1)) * properly center align the loading icon within login/logout button ([#2043](#2043)) ([4fa8ec3](4fa8ec3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This adds terms of service acceptance logging. When a customer accepts the terms of service, we then send
web3.storage-tos-v1
as the agreement value to the API. This then gets stored in a table with a foreign key of the userId.