Bundle function into method

[Nathan-Nesbitt]

Ran into this while I was cleaning up another issue:

https://github.com/SatelCreative/satel-spylib/blob/04cfd44ed8b3684512a76a5799a12f989bf91aae/spylib/oauth/tokens.py#L16-L19

This function is called in only one place; this class:

https://github.com/SatelCreative/satel-spylib/blob/04cfd44ed8b3684512a76a5799a12f989bf91aae/spylib/oauth/tokens.py#L41-L53

This should probably just be a method, as it will reduce the possibility of someone importing it, and it will be easier to read (as a private class method will indicate that only this class uses this method, and it should not be called publicly).

[Nathan-Nesbitt]

@hillairet or would it be better to move this into the main Token class now? As it is a little weird to have this duplicate class just for OAuth.

[hillairet]

Not sure how making this a static method will prevent anyone from using it ... but it's fine as a static method too.
I don't get why it should go into the main Token class. The Token class and its abstract classes goal is to handle the access and storage into a DB. The classes in oauth/tokens.py are meant to get the tokens from Shopify during the OAuth process so different purpose.

[Nathan-Nesbitt]

@hillairet my thought is that we have some redundancy, as we have 2 versions of the same object. Currently this is the flow:

1. We start by obtaining the token (_get_token should actually just be the get method in Token as it's just a function calling another function which just kinda complicates things)

https://github.com/SatelCreative/satel-spylib/blob/3ef58c35e54253f84b0a0793405cdf4c290eee27/spylib/oauth/router.py#L118-L123

2. This is then passed into the post_login function which we define outside the library:

https://github.com/SatelCreative/satel-spylib/blob/3ef58c35e54253f84b0a0793405cdf4c290eee27/spylib/oauth/router.py#L128

3. This outside function normally takes the token, and saves it!

Since we aren't really changing any aspect of the token, I don't see an issue with leveraging the existing Online/Offline Token classes:

https://github.com/SatelCreative/satel-spylib/blob/3ef58c35e54253f84b0a0793405cdf4c290eee27/spylib/token.py#L247-L284

[Nathan-Nesbitt]

I think this was why I was originally thinking attaching the load / save methods to the instance at runtime made sense as we wouldn't have to have the abstract classes (which would be my main concern as this wouldn't follow proper OOP).

[hillairet]

Ok that is clearer.
Indeed the post_login and post_install are probably not needed anymore, unless we want to be able to trigger some process when those happen ... but until that happens we should remove those.
I don't see a problem with having to pass the user's own OfflineTokenDB and OnlineTokenDB classes (that inherit from the abstract classes). This could be validated by mypy before running the code which is a good validation IMO.
I'm happy to discuss different approaches that don't involve attaching stuff at runtime. 😉

[Nathan-Nesbitt]

Maybe it would be preferable to have these as callbacks on the OnlineTokenABC and OfflineTokenABC classes themselves @hillairet ? Maybe something like get_token_callback which is triggered on the get_token call? Something like:

class OnlineTokenABC:
  
  @abstractmethod
  def get_token_callback(self, token: OnlineTokenABC):
    pass

  get_token(cls, domain: str, code: str, api_key: str, api_secret_key: str):
    # Code to obtain token
    token = cls(**params)
    token.update_token(token)

This allows for the user to decide how this is implemented, which in turn allows us to avoid the problems with the current implementation.

[Nathan-Nesbitt]

Because the post_login and post_install are really the callbacks specific to the OnlineToken and OfflineToken classes so we should be able to integrate them into the classes themselves.

The only problem I see is that we need to pass the implementations of the classes OnlineToken and OfflineToken to the router:

https://github.com/SatelCreative/satel-spylib/blob/3ef58c35e54253f84b0a0793405cdf4c290eee27/spylib/oauth/router.py#L25-L37

My thought is that instead of passing post_login and post_install we pass in OnlineToken and OfflineToken which should allow us to avoid implementing any of the abstract classes!

So instead if would look something like this:

def init_oauth_router(
    app_scopes: List[str],
    user_scopes: List[str],
    public_domain: str,
    private_key: str,
    app_handle: str,
    api_key: str,
    api_secret_key: str,
    OnlineToken: OnlineTokenABC,
    OfflineToken: OfflineTokenABC,
    install_init_path='/shopify/auth',
    callback_path='/callback',
) -> APIRouter:

And later on, instead of the following:

https://github.com/SatelCreative/satel-spylib/blob/3ef58c35e54253f84b0a0793405cdf4c290eee27/spylib/oauth/router.py#L118-L123

and

https://github.com/SatelCreative/satel-spylib/blob/3ef58c35e54253f84b0a0793405cdf4c290eee27/spylib/oauth/router.py#L92-L93

We would instead just call:

await OnlineToken.get_token(
    domain=args.shop,
    code=args.code,
    api_key=api_key,
    api_secret_key=api_secret_key
)

[hillairet]

Ok so I really can't see why we would have this get_token part of these classes.
Instead at this line we can just have a simple function:

offline_token = await get_offline_token_from_shopify(

and get rid of post_install and post_login.
The OfflineTokenABC and OnlineTokenABC classes are used to manage the token in and out of a storage of some sort.
Making sure that these classes have a single responsibility is a good practice and we don't have a reason to do otherwise.

[Nathan-Nesbitt]

Okay I will implement it that way @hillairet

[qw-in]

Closing - but good to note for discussion #93