Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SDK-2259:added retrieve qr code #290
SDK-2259:added retrieve qr code #290
Changes from 1 commit
6691dec
be2b96b
6f50c88
fb79e33
1586dea
c2d3b6c
22d0896
359a293
cb5f083
ce57837
f61e496
708fe86
2a0f087
5912800
e4a1fc8
06d91c3
f410402
fd1f342
86d40a4
60e2f4d
7a955f4
33131cc
ae46a53
22bbe7d
9a619e8
bc20038
00ef7b0
4cf2791
ad1f3f9
87d61de
6c2d312
d40c218
f8657c2
4c019b3
a841a3e
87f5098
2492915
5bf052f
9661755
75502ef
b7d7b23
8e20b5e
b893957
4993e35
e52a26c
b9d8fed
c09c2c2
8dd3c7c
f705d44
a4775d2
3da5372
9d403b8
af99524
a3d8314
5f1ec57
3fb2de7
ac64677
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might aswell keep the naming consistent
"ShareSession"
everywhereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the function and the comment use the wrong
id
. This is not thesessionId
but theqrId
from the qr code creation endpointThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
share
? This as instance ofShareSessionFetchedQrCode
should be named accordinlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SOME_QR_CODE_ID
or similiarThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how GO architecture works but I'm pretty sure there is a way to reuse the response obj from the session creation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is, but the session object here is smaller than what's returned for session created - unless the api docs are mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have made this struct inline so i think this is now unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dunno what API u're referring to but both
Session
creation andQR
fetch response (limited to the session) arewhere the full response for the QR fetch endpoint is
would something like the below work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when you fetch the session, the session resource seems to be larger than what is returned in the
fetch qr code
response (what you posted), we could embed it and just have it unmarshal the whole thing, but some extra fields will just be empty for the RP, imo it's better to avoid it if the API itself specifically limits the fields and not give the RP any cause for confusion or incorrect expectations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I thought I answered this but I must have not pushed the button. I meant this session created response