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

Feature/5575 custom auth params for sign up #5577

Merged
merged 14 commits into from
Apr 21, 2022

Conversation

TarasSmakula
Copy link
Contributor

@TarasSmakula TarasSmakula commented Mar 18, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Added currentRegistrationSessionId into RegistrationWizard to add possibility to perform custom sign up stage
  • Added registrationOther into RegistrationWizard to send custom auth params for sign up
  • Moved terms converter into api package to make it accessible in sdk

Motivation and context

#5575

Screenshots / GIFs

Tests

I am using matrix-android-sdk2 in my current project as a local module. Tested different sign up stages on real device and emulator

Tested devices

  • Physical
  • Emulator
  • OS version(s):
    Pixel 3 Android 12

Checklist

@TarasSmakula TarasSmakula changed the title Feature/5575 Feature/5575 custom auth params for sign up Mar 18, 2022
@ouchadam ouchadam added the Z-Community-PR Issue is solved by a community member's PR label Mar 18, 2022
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

Added currentRegistrationSessionId into RegistrationWizard to add possibility to perform custom sign up stage

See my comment, I think this is not necesary to expose the sessionId to the app.

Added registrationOther into RegistrationWizard to send custom auth params for sign up

OK

Moved terms converter into api package to make it accessible in sdk

As far as I understand, converter is not used by the SDK even after the change in this PR. So why do you want to move this code to the SDK?

Last points:

  • can you sign-off your PR else, I will not be able to merge it. Thank!
  • can you add a file for the changelog with extension .sdk ? See the contributing doc to get more info about that.

@TarasSmakula
Copy link
Contributor Author

As far as I understand, converter is not used by the SDK even after the change in this PR. So why do you want to move this code to the SDK?

Yep, just made this mapping accessible for others sdk users. Using this in my current project to build terms list.
So i think it will be good to have this publicly available in sdk.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

changelog.d/5575.sdk Outdated Show resolved Hide resolved
changelog.d/5575.sdk Outdated Show resolved Hide resolved
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for your update. There is still some renaming to do. Also there is a conflict to fix, can you handle it please?

Once done, I think we will be able to merge your PR. Thanks!

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

@bmarty bmarty enabled auto-merge (squash) April 21, 2022 19:22
@bmarty bmarty merged commit 2839d14 into element-hq:develop Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants