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

redirection phone or email #225

Merged
merged 19 commits into from
May 28, 2021
Merged

Conversation

filippo888
Copy link
Collaborator

Content

This PR refer to the issue #213. I modify the simple user activity for allowing the user contact the announcer via phone or email. If the phone number is not present when we click the contact user button, you will directly redirected on the mail service else you will ask via a Alert dialog if you prefer contact the announcer via phone or via email.

Demo

Registrazione.schermo.2021-05-27.alle.16.41.09.mov

@codeclimate
Copy link

codeclimate bot commented May 28, 2021

Code Climate has analyzed commit e659be6 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 30.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 83.4% (-0.3% change).

View more on Code Climate.

Copy link
Collaborator

@CaiMusso CaiMusso left a comment

Choose a reason for hiding this comment

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

Good to go! Thanks for remembering the fixes in the layout of activity_filter on text size format of PR #220. I left a comment but will be addressed in the general refactor next week.

public void openEmailOrPhone(View view){
if(!advertiserUser.getPhoneNumber().isEmpty()) {
AlertDialog.Builder builder = new AlertDialog.Builder(this);
builder.setTitle("How did you prefer contact the announcer ?");
Copy link
Collaborator

Choose a reason for hiding this comment

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

In refactor PR next week this sentence should be changed to present. And maybe something shorter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes absolutely!

Copy link
Collaborator

@rovati rovati left a comment

Choose a reason for hiding this comment

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

Good PR, just a small comment about the permissions code that we could move completely to PermissionRequest at a later time

Comment on lines 125 to 131
int permissionCheck = ContextCompat.checkSelfPermission(this, Manifest.permission.CALL_PHONE);
if (permissionCheck != PackageManager.PERMISSION_GRANTED) {
ActivityCompat.requestPermissions(
this,
new String[]{Manifest.permission.CALL_PHONE},
123);
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe in a refactor PR we could modify this code to follow the same way it is structured in other cases where we ask for permissions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch @rovati ! This should be implemented in the PermissionRequest. Other thing to add to the final refactor.

@CaiMusso CaiMusso merged commit 7b805ea into master May 28, 2021
@CaiMusso CaiMusso deleted the feature/redirection-phone-or-email branch May 28, 2021 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants