Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/pardner/services/tumblr.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from typing import Any, Iterable, Optional

from pardner.services import BaseTransferService
from pardner.stateless.tumblr import URLs

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It might get confusing if stateless/ and services/ are importing form each other, but I think it's fine as long as only services/ is pulling from stateless/ and not vice-versa.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

got it that makes sense

from pardner.verticals import Vertical


Expand All @@ -11,8 +12,8 @@ class TumblrTransferService(BaseTransferService):
See API documentation: https://www.tumblr.com/docs/en/api/v2
"""

_authorization_url = 'https://www.tumblr.com/oauth2/authorize'
_token_url = 'https://api.tumblr.com/v2/oauth2/token'
_authorization_url = URLs.AuthorizationURL
_token_url = URLs.TokenURL

def __init__(
self,
Expand Down
72 changes: 72 additions & 0 deletions src/pardner/stateless/tumblr.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
from enum import StrEnum
from typing import Any, Iterable, Optional

from pardner.stateless import Scope
from pardner.stateless.base import (
generic_construct_authorization_url,
generic_fetch_token,
)
from pardner.verticals import Vertical


class URLs(StrEnum):
AuthorizationURL = 'https://www.tumblr.com/oauth2/authorize'
TokenURL = 'https://api.tumblr.com/v2/oauth2/token'


def scope_for_verticals(verticals: Iterable[Vertical]) -> set[str]:
# Tumblr only needs 'base' for read access requests
return {'base'}


def construct_authorization_url(
client_id: str, redirect_uri: str, scope: Scope = {'base'}
) -> tuple[str, str]:
"""
Builds the authorization URL and state for Tumblr.

:param client_id: Client identifier given by the OAuth provider upon registration.
:param redirect_uri: The registered callback URI.
:param scope: The scope of the access request. These may be any string but are
commonly URIs or various categories such as ``videos`` or ``documents``.

:returns: the authorization URL and state, respectively.
"""
return generic_construct_authorization_url(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we do a lot of these services wouldn't it be clean to use class inheritance to automatically inherit the construct_authorization_url, and then wrap it and call super()... if the method even needs to be overridden?

This is just meant as food for thought, one often wants to do things 3 times before abstracting or generalizing... we might want to see how the data methods interact before rearchictecting

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think what you're suggesting is more or less how I set things up in the "stateful" model (e.g., BaseTransferService.fetch_token and TumblrTransferService.fetch_token).

Maybe it's not worth creating stateless versions of those methods after all and just use the classes + methods that already exists for the stateless case as well. If we are using classes even in the stateless mode, the work is pretty much already done (see below)! The reason I was hesitant to use classes for the stateless case is that the ___TransferService object would be used just one time to call a method on it. But now that I'm thinking about it, we could reuse that same object after getting back the token to make the transfer requests.

What I propose: revert #34, close this PR, and use the existing classes (BaseTransferService and TumblrTransferService) for the stateless use case. It reduces code duplication and achieves the same thing in a slightly different way as the functions in this PR and #34 .

UX with classes

initiate oauth2

from pardner.services import TumblrTransferService
tumblr = TumblrTransferService('client_id', 'client_secret', 'https://redirect.com', [Vertical.FeedPost])
auth_url, state = tumblr.authorization_url()
# forward user to auth_url

callback url

from pardner.services import TumblrTransferService
# need to create new instance because the other one is in a completely different scope
tumblr = TumblrTransferService('client_id', 'client_secret', 'https://redirect.com', [Vertical.FeedPost])
token = tumblr.authorization_url(code = '39040239402')

UX without classes

initiate oauth2

from pardner.stateless.tumblr import construct_authorization_url
auth_url, state = construct_authorization_url('client_id', 'https://redirect.com', scope = {'base'))
# forward user to auth_url

callback url

from pardner.stateless.tumblr import fetch_token
token = fetch_token('client_id', 'https://redirect.com', client_secret = 'client_secret', code = '39040239402')

@lisad lisad Jul 25, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, this is very good and I love the thinking - but also I think about continuing it just a little further, and calling the methods to fetch comments, or fetch block list

Editing to pull the tumblr context out of storage since this gets triggered AFTER initial setup... and I realize I might not be thinking enough about what parts of this happen in different segments of the process separated by different triggers and server contexts. We might not want the OAuth setup that gets triggered by the first click of the "donate my data" button, to be the same object that gets triggered when the callback URL receives a go-ahead token.

tumblr = TumblrTransferService(token=TokenStorage.get(username=username))
tumblr.fetch_block_list()
tumblr.fetch_comments(since=timezone.now() - relativedelta(days=1))
tumblr.fetch_user_profile()

vs

TumblrTransferService.fetch_block_list(token, username, etc)
TumblrTransferService.fetch_comments(token, username, since=etc)
TumblrTransferService.fetch_user_profile(...)

@aborem aborem Jul 28, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think we're on the same page about definiting the data-specific methods; what you've laid out is pretty much exactly what I was thinking of doing!

So in addition to defining traditional methods that can be called on instances of the TumblrTransferService class, I'll also define static class methods that do pretty much the same thing but without using any instance attributes. → I don't think this is necessary, it would involve some strange antipatterns (like writing the same method twice: as a classmethod and a normal method).

URLs.AuthorizationURL, client_id, redirect_uri, scope
)


def fetch_token(
client_id: str,
redirect_uri: str,
authorization_response: Optional[str] = None,
client_secret: Optional[str] = None,
code: Optional[str] = None,
) -> dict[str, Any]:
"""
Makes a request to Tumblr's resource server to obtain the access token.

One of either `code` or `authorization_response` must not be None.

:param client_id: Client identifier given by the OAuth provider upon registration.
:param redirect_uri: The registered callback URI.
:param scope: The scope of the access request. These may be any string but are
commonly URIs or various categories such as ``videos`` or ``documents``.
:param authorization_response: the URL (with parameters) the end-user's browser
redirected to after authorization.
:param client_secret: The `client_secret` paired to the `client_id`.
:param code: Authorization code (used by WebApplicationClients).

:returns: the authorization URL and state, respectively.
"""
return generic_fetch_token(
client_id,
redirect_uri,
{'base'},
URLs.TokenURL,
authorization_response,
client_secret,
code,
include_client_id=True,
)
44 changes: 44 additions & 0 deletions tests/test_stateless/test_tumblr.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
from pardner.stateless.tumblr import (
URLs,
construct_authorization_url,
fetch_token,
scope_for_verticals,
)
from pardner.verticals import Vertical
from tests.conftest import get_url_params


def test_scope_for_verticals():
assert scope_for_verticals({Vertical.FeedPost}) == {'base'}


def test_construct_authorization_url():
auth_url, state = construct_authorization_url(
'fake_client_id', 'https://redirect_uri'
)
assert auth_url.startswith(URLs.AuthorizationURL)

auth_url_params = get_url_params(auth_url)

assert 'client_id' in auth_url_params
assert auth_url_params['client_id'] == 'fake_client_id'
assert 'redirect_uri' in auth_url_params
assert auth_url_params['redirect_uri'] == 'https://redirect_uri'
assert 'state' in auth_url_params
assert auth_url_params['state'] == state
assert 'scope' in auth_url_params
assert 'base' in auth_url_params['scope']


def test_fetch_token_with_code(mock_outbound_requests):
mock_oauth2session_request, mock_client_parse_request_body_response = (
mock_outbound_requests
)
fetch_token(
'fake_client_id',
'https://redirect_uri',
client_secret='fake client secret',
code='the_best_code',
)
mock_oauth2session_request.assert_called_once()
mock_client_parse_request_body_response.assert_called_once()