Skip to content

ECWID-161269 - add a new modified RequestKind to replace ApiCredentials #485

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ArAkAru
Copy link
Contributor

@ArAkAru ArAkAru commented Apr 16, 2025

ECWID-161269

  • create new abstract RequestKind to replace ApiCredentials
  • make ApiCredentials filed as optional with default null in public ApiClientHelper constructor (for future deletion)

@ArAkAru ArAkAru self-assigned this Apr 16, 2025
val headers = headers.withRequestId(requestId).withCredentials(credentials)

val headers = headers.withRequestId(requestId).let {
if (requestKind != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If requestKind is provided, we use all data from it; otherwise, we use the old data from credentials.
all this if block will be deleted soon

generalInfo = UpdatedStoreProfile.GeneralInfo(
storeUrl = ""
),
generalInfo = UpdatedStoreProfile.GeneralInfo(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this thing breaks the behavior in tests because the platform is not IS and therefore we get dirty/hash urls

@ArAkAru ArAkAru marked this pull request as ready for review April 21, 2025 14:13
@ArAkAru ArAkAru requested a review from Alexis2004 April 21, 2025 14:13
@@ -0,0 +1,14 @@
package com.ecwid.apiclient.v3.config

class ApiV3InstantSiteRequestKind(
Copy link
Contributor

Choose a reason for hiding this comment

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

Это же решили в основной клиент не добавлять пока, а оставить в приватном?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

не очень понял, какой приватный?
и почему не добавляем в основной?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

а, имеешь ввиду реализации сделать в extended либе?
а тут оставить только RequestKind

private val apiToken: String,
) : RequestKind() {
override fun buildBaseEndpointPath(): String {
return "/instantsite/api/v1/$storeId"
Copy link
Contributor

Choose a reason for hiding this comment

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

А почему SID тут е нужен? Как будет работаь шардирование?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

дело в том, что есть запрос на получение токена и он выглядит как
instantsite/api/v1/oauth/token - тут нету SID
SID решил вынести в само формирование запроса:
https://github.com/Ecwid/ecwid/blob/e03dee87ab53daa9d7908f93f487466cf1c02f23/libs/apiclientext/src/main/kotlin/com/ecwid/apiclientext/v3/instantsite/request/InstantsiteProfileRequest.kt

а здесь оставить только общую часть

Copy link
Contributor

Choose a reason for hiding this comment

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

Не, не нужно так делать. Лучше сделать 2 типа запросов с разными RequestKind'ами.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

оке, я сделаю так тогда в коде проекта, здесь я удалил имплементации

@ArAkAru ArAkAru requested a review from Alexis2004 April 23, 2025 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants