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

New getTopazValues method #142

Closed
aquiros20 opened this issue Jul 23, 2024 · 10 comments
Closed

New getTopazValues method #142

aquiros20 opened this issue Jul 23, 2024 · 10 comments
Assignees

Comments

@aquiros20
Copy link

aquiros20 commented Jul 23, 2024

From Topaz SDK we need to obtain a new value and make it available via a new bridge method.
The value is "syncId"
There's already a method that returns a value from Topaz SDK (token) so the behavior should be similar.

The proposed method is as follows:
getTopazValues = () => Promise<{syncId: string}>

This proposal includes a more generic name in order to allow it to return other Topaz values and not only the sync id, in case in the future we require more from Topaz sdk, not to fall in the current situation where the existing method (getTopazToken) is pretty specific. But this part is just a suggestion.

UPDATE: After discussion, the method signature will end up being implemented as follows:

getTopazValues = () => Promise<{syncId?: string}>

Which covers the possible response scenarios:

  • {"syncId":"Value"}
  • {"syncId":""}
  • {}
@WanaldinoTelefonica
Copy link

WanaldinoTelefonica commented Aug 28, 2024

Hi @aquiros20

We purpose the following values.

  • For the bridge function name we recomend "GET_TOPAZ_VALUES"
  • The payload will be something like:
{
    syncId: String?
}

That value can be null as the SDK can return this value.

@marcoskolodny
Copy link
Contributor

Why not deprecating getTopazToken method and including the token in the getTopazValues payload?

@pladaria
Copy link
Member

pladaria commented Aug 30, 2024

Agree witn @marcoskolodny

The new method could be:

const getTopazInfo = (options: {timeout?: number} = {}) => Promise<{token: string, syncId: string}>

And the old one (getTopazToken) would be deprecated and just an alias that returns the same as getTopazInfo:

/** @deprecated **
const getTopazToken = getTopazInfo;

Some questions:

  • Should we keep the timeout option?
  • Are the response fields optional?
  • In case of timeout, should we return null values or respond with an error?

@WanaldinoTelefonica
Copy link

That a good approach but sync_id is a value that we only need the SDK to start to fetch and token is an async value that we cannot control how much it would need to be ready. Moving token into this method can make it quite slow.

Answering @pladaria questions:

  • I haven't seen anywhere the timeout is used
  • Fields should be optional, that is what the SDK returns
  • Thats a problem if some of the values your fetching are failures and some other don't. In this case we can set as null each value that failed, but you cannot know what happened, or we can add booleans in the model to set if some value failed.

I would focus this issue on creating the new method to fetch the syncID and maybe we can start an other issue to improve that same method to add more features.

@pmartinbTEF
Copy link
Contributor

pmartinbTEF commented Sep 2, 2024

I think it is better to maintain two separated calls in order to simplify the error management.
@pladaria In the login case the app have a timeout of 5 seconds for obtaining the syncID because the login button is disabled until syncId is obtained. In the case of this new bridge method we have not set any timeout.
By now the messages that the app can send to Web when requesting the syncId could be the following:

{"type":"GET_TOPAZ_VALUES","id":"0","payload":{"syncId":"Value"}} /Non empty SyncId/
{"type":"GET_TOPAZ_VALUES","id":"0","payload":{"syncId":""}} /Empty SyncId/
{"type":"GET_TOPAZ_VALUES","id":"0","payload":{}} /Null syncId/
{"type":"ERROR","id":"0","payload":{"code":500,"description":"The operation couldn’t be completed."}} /Error because Topaz SDK couldn't be initialized/

@dpastor
Copy link
Contributor

dpastor commented Sep 2, 2024

That a good approach but sync_id is a value that we only need the SDK to start to fetch and token is an async value that we cannot control how much it would need to be ready. Moving token into this method can make it quite slow.

Answering @pladaria questions:

  • I haven't seen anywhere the timeout is used
  • Fields should be optional, that is what the SDK returns
  • Thats a problem if some of the values your fetching are failures and some other don't. In this case we can set as null each value that failed, but you cannot know what happened, or we can add booleans in the model to set if some value failed.

I would focus this issue on creating the new method to fetch the syncID and maybe we can start an other issue to improve that same method to add more features.

With other bridge methods (Specially the ones related to eSim information fetch), we had similar discussions about having one or more bridge methods. IMO more granular bridge methods are always easier to implement, simpler, faster, and can have specific error responses for the specific information being requested. Also, as @WanaldinoTelefonica mentioned, if in this case, information is available in different moments, and error management could be complex as @pmartinbTEF mentions, so, these not be grouped, no?

@pladaria
Copy link
Member

pladaria commented Sep 2, 2024

Ok, let's go with a new method.

@aquiros20, please update the ticket description setting the sync_id as optional/nullable

@pmartinbTEF regarding this:

{"type":"GET_TOPAZ_VALUES","id":"0","payload":{"syncId":""}} /Empty SyncId/

what's the difference between an empty syncId and a null one? Can we unify both cases as payload: {}?

@WanaldinoTelefonica
Copy link

I don't know if an empty syncID is the same as a nul one but we can agree that both are not an ID so we can omit from the payload if everyone is agree.

@pladaria
Copy link
Member

pladaria commented Sep 3, 2024

ok, LGTM
@aquiros20, please update the issue description with the final proposal that should be implemented

@aquiros20
Copy link
Author

ok, LGTM @aquiros20, please update the issue description with the final proposal that should be implemented

Done.

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

No branches or pull requests

7 participants