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

Removed auto-populating phone numbers #5481

Merged
merged 32 commits into from
Apr 12, 2023
Merged

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Mar 9, 2023

Closes #5482

What has been done to verify that this works as intended?

I've added automated tests and tested the fix manually.

Why is this the best possible solution? Were any other approaches considered?

Please test metadata using the form attached to the issue. There was a lot of refactoring there so everything related to it might have been affected.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

I've implemented a lot of refactoring so please test all metadata questions. Please also read the automated test because it might be helpful.

Do we need any specific form for testing your changes? If so, please attach one.

The form attached to the issue.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Yes getodk/docs#1596

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 marked this pull request as ready for review March 9, 2023 19:11
@grzesiek2010 grzesiek2010 changed the title Property fix Fix reading the device's phone number Mar 9, 2023
@grzesiek2010 grzesiek2010 force-pushed the property branch 3 times, most recently from a632a40 to bb50576 Compare March 14, 2023 18:38
@seadowg seadowg added needs testing high priority Should be looked at before other PRs/issues labels Mar 21, 2023
@dbemke
Copy link

dbemke commented Mar 22, 2023

I checked it on Android 10, 11, 13 (with a sim card) and in settings(user and device identity) and metadata form the phone number isn't visible.

@grzesiek2010
Copy link
Member Author

Wow, I've tested it on emulators so everything was fine but I can confirm it's not always the case with real devices. One problem is that on newer devices an exception might be thrown which I could handle with the fix we already have in #5483.
But even if I do that an empty string is returned instead of the number I want. I've tried the current implementation with TelephonyManager, and also another one with SubscriptionManager and it's the same.
Browsing StackOverflow I see that people claim there is no one perfect and reliable solution to read a phone number.
Taking that into account and the fact that it has been broken for more than 2 years and no one has complained, maybe we should remove that at all and just let users enter their numbers manually? @lognaturel @seadowg what do you think?

@seadowg
Copy link
Member

seadowg commented Mar 23, 2023

I'm going to move the issue over to v2023.2 and make this PR a draft. We can move the discussion over to the issue.

@seadowg seadowg marked this pull request as draft March 23, 2023 10:22
@grzesiek2010 grzesiek2010 removed high priority Should be looked at before other PRs/issues needs testing labels Mar 23, 2023
@grzesiek2010 grzesiek2010 changed the title Fix reading the device's phone number Removed auto-populating phone numbers Apr 5, 2023
@grzesiek2010 grzesiek2010 marked this pull request as ready for review April 5, 2023 13:12
@dbemke
Copy link

dbemke commented Apr 7, 2023

If the user fills the user-defined metadata values in settings and later on removes them, the values are still present in the metadata form opened after removing the values. If the user kills the app, the values won’t be visible in the metadata form any more.
The issue is not present on the master version.

Steps to reproduce:

  1. Go to settings→ user and device identity→ form metadata.
  2. Fill the username, phone number, email address
  3. Remove the filled values so that they are "Not set”.
  4. Go to the metadata form and check if the user data is not set.
    Metadata new.xlsx.txt

@grzesiek2010
Copy link
Member Author

Good catch. The issue has been fixed.

@srujner
Copy link

srujner commented Apr 11, 2023

Read phone state permission appeared again.
The issue is present on device with Android 13 also on the Master version

Steps to reproduce:

  1. Install ODK application;
  2. Open ODK app;
  3. Go to the metadata form;
    Metadata new.xlsx.txt
  4. Phone permission will appear.
    Screenshot_20230411-170615
  5. Click Don't allow.
    Screenshot_20230411-170622

@grzesiek2010
Copy link
Member Author

This seems impossible, I've checked Android 13... I think it was an installation error and you ended up testing the master branch thinking it's this branch. Please uninstall the app completly and do everything again.

@srujner
Copy link

srujner commented Apr 12, 2023

Correct, something was wrong with my branch. False alarm ;)

@dbemke
Copy link

dbemke commented Apr 12, 2023

Tested with Success!

Verified on device with Android 8.1, 10

Verified cases:

  • issue The device's phone number can't be read #5482
  • checking metadata in an xml file of a saved form
  • checking metadata displayed in a fieldlist in the metadata form
  • adding/ removing user-defined form metadata
  • setting a username in server settings and form metadata settings
  • validating an email address
  • background location in a form with metadata
  • regression checks in User and Device identity
  • dial number form

@srujner
Copy link

srujner commented Apr 12, 2023

Tested with Success!

Verified on device with Android 12, 13

@grzesiek2010 grzesiek2010 merged commit 9f37761 into getodk:master Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The device's phone number can't be read
4 participants