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

User data model #10

Merged
merged 7 commits into from
May 28, 2020
Merged

User data model #10

merged 7 commits into from
May 28, 2020

Conversation

rocconnick
Copy link
Collaborator

This PR adds basic object-mapping functionality for Cloudant and user representation.

A generic type is represented by the models.Document base class. This provides utilities for accessing a specific document or all of a given type. Subclasses specify a type and define attributes.

@rocconnick
Copy link
Collaborator Author

This is still a work in progress. I need to integrate it into the segmund and strava modules.

@jglynn
Copy link
Owner

jglynn commented May 27, 2020

Looks good & running clean on my local. Shall I merge it @rocconnick ?

@rocconnick
Copy link
Collaborator Author

Let's not merge it yet. I left a route up that would expose sensitive sensitive tokens if deployed. I'm going to integrate it into the strava and segmund modules tonight, and take away the route I was using for testing.

@rocconnick
Copy link
Collaborator Author

rocconnick commented May 28, 2020

This is ready now! I clicked around the page and everything seems to be working. I also tested the user registration by deleting myself from the DB and re-registering.


def exists(self):
"""Return True if this document exists in database."""
return User.contains(self._id)
Copy link
Owner

Choose a reason for hiding this comment

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

Should this use the metadata class a la Document.contains(self._id)?

With that said, since _id is unique to all documents I'm sure this User.contains will return the same result.

I see a couple other user variable name leftovers from stuff it looks like you were abstracting out. Not causing any issues, we can clean those up later for clarity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Submitting a separate PR now.

This worked because User didn't implement that method, so really it was the same thing. It could have broken down the road. And the other references were just variable names, but sloppy nonetheless.

# this use the current user's token? Maybe that would be slightly
# safer for rate limits?
john = models.User.get('326452')
johns_token = self.get_user_access_token(john)
Copy link
Owner

Choose a reason for hiding this comment

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

In order to leverage the user's token we would need to authenticate them so we knew who they were. They'll only be registering once so we can't really rely on that to initiate a session/etc reliably. Were you thinking of some sort of approach in particular?

I think a good approach would be to use the token that was issued to my API Account Segmund. The refresh token is currently stored in config.json and we would just need to keep the auth token and expiration time somewhere. It looks like the rate limiting is tied to the Segmund app anyways regardless of what token we're using, likely via client_id. I can see the rate usage data in Strava.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. They're only registering once, but we check the validity of the access token on every request and get new tokens if necessary. Assuming it's a user driving the browser, aren't we guaranteed to have a good set of tokens?

Things that happen without user interaction should definitely use the application refresh token.

I could be wrong here. Every time I think I understand OAuth, I find out I don't actually understand it.


user = strava_service.register_user(auth_code)

# TODO: remove this condition? I don't think the function can return None
Copy link
Owner

Choose a reason for hiding this comment

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

Agree.

It looks like register_user returns a model object and we'll likely always want to call save below? The registration process is idempotent so if a user does it again I think we get a new token and may want to persist. I haven't dug in to see if Strava is smart enough to issue new tokens and expire the old ones in this scenario.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right that we should persist the new one if a user re-registers. If the API generates new tokens, we should use those ones.

Copy link
Owner

@jglynn jglynn left a comment

Choose a reason for hiding this comment

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

Looking good!

Made a few comments along the way - no blockers - will merge now and we can chat over email.

@jglynn jglynn merged commit ebf2e92 into master May 28, 2020
@jglynn
Copy link
Owner

jglynn commented May 28, 2020

IBM Cloud toolchain: Delivery Pipeline deployed segmund to dev, including commit ebf2e92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants