[74542] Implement PageInfo query#23092
Conversation
DRY code a bit. Updated `BaseQuery` as well as `UserQuery`.
Fix specs: no provider needed for now.
…external-wiki-providers
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
| class PageInfoContract < DryApplicationContract | ||
| params do | ||
| required(:identifier).filled(:string) | ||
| optional(:access_token).maybe(:string) |
There was a problem hiding this comment.
When I read the headline of the PR, I suspected that I would find exactly this in here 🙈
Is the current action plan to temporarily expect this optional parameter and work on a proper implementation of authorization strategies as the immediate next thing?
The pain that I have with reviewing this is that there will be a lot of code in the wrong place right now and we need to be aligned that this code is planned to be moved to the right place.
The alternative would be to leave this PR in draft state and tackle authentication first, then return here to finish the querying off properly.
| OAuthClientToken.for_user_and_client(user, oauth_client).exists? | ||
| end | ||
|
|
||
| def user_access_token(user) |
There was a problem hiding this comment.
To give one example why I am skeptical of the approach of merging this PR first and then taking care of authentication: This is a method that's now added to the public interface of every provider.
In my mind, this method has to be removed in a subsequent PR and it's existence is only temporary. But I have no clue whether this is how you think about this method. You might want to keep it and then one of two things would happen at a later point:
- I forget about this method and don't even mention it anymore; I will be very sad once I rediscover it
- I don't forget about this method and ask in the 2nd PR why it's not gone; You will be very sad because you thought we were aligned on the path forward and I already approved this method in PR 1
|
I'm converting this PR to draft until the auth strategy is implemented 👨🏼💻 |
2 participants
Ticket
74542
What are you trying to accomplish?
Implement
PageInfoquery. DRY code a bit. UpdatedBaseQueryas well asUserQuery.Rename
UserQuerytoUser.Merge checklist