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: Custom server no network dialog #WPB-11627 #3543

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

m-zagorski
Copy link
Contributor

@m-zagorski m-zagorski commented Oct 23, 2024

TaskWPB-11627 [Android] Show information to user when he tries to login, but no network is available

https://wearezeta.atlassian.net/browse/WPB-11627

What's new in this PR?

Issues

When we're trying to connect with custom server and we didnt have internet connection, we didnt display correct popup

Solutions

Due to the matter of handling invalid json being exactly the same as no network, we have to check on our own if we're connected to internet or not. Depending on that we display no network dialog.

Testing

How to Test

Attachments (Optional)

Screenshot_20241023_073149_Wire Beta


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Oct 23, 2024
Copy link

sonarcloud bot commented Oct 23, 2024

@@ -122,7 125,8 @@ class WireActivityViewModel @Inject constructor(
private val observeScreenshotCensoringConfigUseCaseProviderFactory: ObserveScreenshotCensoringConfigUseCaseProvider.Factory,
private val globalDataStore: Lazy<GlobalDataStore>,
private val observeIfE2EIRequiredDuringLoginUseCaseProviderFactory: ObserveIfE2EIRequiredDuringLoginUseCaseProvider.Factory,
private val workManager: Lazy<WorkManager>
private val workManager: Lazy<WorkManager>,
private val networkStateObserver: Lazy<NetworkStateObserver>
Copy link
Member

Choose a reason for hiding this comment

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

this is over kill you don't need to check the network state, we can do the request to get the json with custom server meta data and check the error returned it is a network error with something list Io Exception then display this error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is unfortunately not doable:

  • When we dont have internet connection and the config is valid we're getting:
    GenericError(cause=java.net.UnknownHostException: Unable to resolve host "nginz-https.bund-next-column-offline-android.wire.link": No address associated with hostname)

  • When we have internet connection and the config is invalid we're getting:
    GenericError(cause=java.net.UnknownHostException: Unable to resolve host "nginz-https.unreachable.wire.link": No address associated with hostname)

So we're unable to know if the config was wrong or not with the exception we're getting

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could slightly change the copy and use this dialog in both cases?
Like: "You don't seem to be connected to the Internet or the config link you used is broken. Please check your Internet connection and try again." 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well yeah we could have only one for malformed json and no internet connection :D that'd solve the issue too

Copy link
Member

Choose a reason for hiding this comment

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

When we dont have internet connection and the config is valid we're getting:
GenericError(cause=java.net.UnknownHostException: Unable to resolve host "nginz-https.bund-next-column-offline-android.wire.link": No address associated with hostname)

I feel like i am missing something?
this will never happen how was the app able to know that the constant is valid if we are not able to read the content, since there is no internet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes pushed as we talked today, we now have the no network message for both cases ;)

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.77%. Comparing base (d5b89ca) to head (ac5e31e).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3543    /-   ##
========================================
  Coverage    45.77%   45.77%           
========================================
  Files          472      472           
  Lines        16024    16025     1     
  Branches      2705     2705           
========================================
  Hits          7335     7336     1     
  Misses        7928     7928           
  Partials       761      761           
Files with missing lines Coverage Δ
...rc/main/kotlin/com/wire/android/ui/WireActivity.kt 0.00% <ø> (ø)
...otlin/com/wire/android/ui/WireActivityViewModel.kt 71.42% <100.00%> ( 0.09%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5b89ca...ac5e31e. Read the comment docs.

@m-zagorski m-zagorski changed the title Custom server no network dialog feat: Custom server no network dialog #WPB-11627 Oct 23, 2024
Copy link
Contributor

Built wire-android-staging-compat-pr-3543.apk is available for download

Copy link
Contributor

Built wire-android-dev-debug-pr-3543.apk is available for download

Copy link

sonarcloud bot commented Dec 2, 2024

Copy link
Contributor

github-actions bot commented Dec 2, 2024

Built wire-android-staging-compat-pr-3543.apk is available for download

Copy link
Contributor

github-actions bot commented Dec 2, 2024

Built wire-android-dev-debug-pr-3543.apk is available for download

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: unplanned Any work item that isn’t part of the product or technical roadmap. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants