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

feat: use the raw homeserver URL instead of manually removing the scheme #1382

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Sep 19, 2023

On the one hand, the SDK knows what to do and will clean up the URL if needs be, guessing what the best scheme (http or https) is based on the prefix. On the other hand, trimming the scheme as done here doesn't allow for manual testing on insecure servers, which is handy in e2e testing situations.

@bnjbvr bnjbvr requested a review from a team as a code owner September 19, 2023 16:48
@bnjbvr bnjbvr requested review from julioromano and removed request for a team September 19, 2023 16:48
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage: 50.00% and no project coverage change.

Comparison is base (d784ca3) 57.78% compared to head (ffb1302) 57.78%.
Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1382   +/-   ##
========================================
  Coverage    57.78%   57.78%           
========================================
  Files         1124     1124           
  Lines        29593    29591    -2     
  Branches      6032     6030    -2     
========================================
  Hits         17100    17100           
  Misses        9912     9912           
+ Partials      2581     2579    -2     
Files Changed Coverage Δ
...s/login/impl/changeserver/ChangeServerPresenter.kt 80.00% <0.00%> (+3.80%) ⬆️
...accountprovider/ConfirmAccountProviderPresenter.kt 89.47% <100.00%> (+1.54%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

On the one hand, the SDK knows what to do and will clean up the URL if needs be,
guessing what the best scheme (http or https) is based on the prefix. On the other
hand, trimming the scheme as done here doesn't allow for manual testing on insecure
servers, which is handy in e2e testing situations.
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.

LGTM, thanks. Not sure why we try to extract the domain from the url. Maybe influenced by the parameter name of configureHomeserver. This is serverNameOrHomeserverUrl now, but I think it was something else before.

@bmarty bmarty merged commit aacef46 into element-hq:develop Sep 19, 2023
11 of 14 checks passed
@bnjbvr bnjbvr deleted the use-raw-homeserver-url branch September 21, 2023 09:19
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