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

Fixes #209 : Autocomplete email text input #217

Merged
merged 1 commit into from Jun 23, 2017

Conversation

4 participants
@rohanagarwal94
Copy link
Contributor

rohanagarwal94 commented Jun 21, 2017

Fixes #209

screenshot_2017-06-21-23-04-48-643_org fossasia openevent app

String password = etPassword.getText().toString();

presenter.login(email, password);
});

savedEmails = new HashSet<>();
sharedPrefs = PreferenceManager.getDefaultSharedPreferences(this);

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 21, 2017

Member

We don't add logic in views. Please use UtilModel and LoginPresenter. Also write appropriate unit tests.

Thanks

This comment has been minimized.

@rohanagarwal94

rohanagarwal94 Jun 21, 2017

Contributor

Unit tests in the same PR?

This comment has been minimized.

@iamareebjamal
@rohanagarwal94

This comment has been minimized.

Copy link
Contributor

rohanagarwal94 commented Jun 21, 2017

@iamareebjamal @mejariamol please review.

@mejariamol

This comment has been minimized.

Copy link
Member

mejariamol commented Jun 21, 2017

@rohanagarwal94 please add fixes #[Issue] in the comment.

@rohanagarwal94 rohanagarwal94 force-pushed the rohanagarwal94:autocomplete branch from 88a9f35 to 66f7303 Jun 22, 2017

@rohanagarwal94

This comment has been minimized.

Copy link
Contributor

rohanagarwal94 commented Jun 22, 2017

@iamareebjamal @mejariamol please review now. Will resolve the conflicts once approved.

String password = etPassword.getText().toString();

presenter.login(email, password);
});

if(presenter.getEmailList() != null)

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 22, 2017

Member

Don't do this. Create a presenter start method, and it will call attachEmails on the view, if it has emails, or else, it won't

@@ -91,6 +101,8 @@ public void showProgressBar(boolean show) {

@Override
public void onLoginSuccess() {
if(etEmail.getEditText().getText() != null)
presenter.saveEmail(etEmail.getEditText().getText().toString());

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 22, 2017

Member

Similarly here, take this action in presenter in the login callback, if it is successful. We add all logic to presenter so that we can unit test it

@@ -136,4 +144,12 @@ public void shouldNotAccessView() {
Mockito.verifyNoMoreInteractions(loginView);
}

@Test
public void shouldGetEmailListSuccessfully() {

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 22, 2017

Member

This test is actually like testing getter and setter. We test flow of information among view and presenter

@rohanagarwal94

This comment has been minimized.

Copy link
Contributor

rohanagarwal94 commented Jun 23, 2017

@iamareebjamal @mejariamol please review now.

@@ -59,7 +66,7 @@ protected void onCreate(Bundle savedInstanceState) {

btnLogin.setOnClickListener(v -> {

String email = etEmail.getText().toString();
String email = etEmail.getEditText().getText().toString();

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

Any specific reason to change this

This comment has been minimized.

@rohanagarwal94

rohanagarwal94 Jun 23, 2017

Contributor

Yes, to include the autocomplete text view. To get the text from the textinputlayout, edittext needs to be called first.

This comment has been minimized.

@iamareebjamal
if(loginView != null && loginModel.isLoggedIn())
loginView.onLoginSuccess();

if(loginView != null && getEmailList() != null)

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

These lines are adjacent. So, move null check on top and return if it is null or logged in and only check for email list being null before attaching them

@Before
public void setUp() {
loginPresenter = new LoginPresenter(loginModel);
loginPresenter = new LoginPresenter(loginModel, utilModel);
loginPresenter.saveEmail(email);

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

Why saving deliberately?

@@ -136,4 +144,19 @@ public void shouldNotAccessView() {
Mockito.verifyNoMoreInteractions(loginView);
}

@Test
public void shouldAttachEmailOnLoginSuccessfully() {

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

Good work, also test that emails are attached automatically if present and not attached if they are not present. So, 2 more tests

@rohanagarwal94

This comment has been minimized.

Copy link
Contributor

rohanagarwal94 commented Jun 23, 2017

@iamareebjamal done. Please suggest any other changes before I resolve conflicts.


@Override
public void addStringSetElement(String key, String value) {
Set<String> set = sharedPreferences.getStringSet(key, new HashSet<>());

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

A minor change required here, use the getStringSet of UtilModel, so that there is less dependence on external dependencies

if(loginView == null)
return;

if(loginModel.isLoggedIn())

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

If it is logged in, just call on success and return, no need to attach emails then

This comment has been minimized.

@rohanagarwal94

rohanagarwal94 Jun 23, 2017

Contributor

Ah my mistake.


@Test
public void shouldAttachEmailAutomatically() {
Mockito.when(utilModel.getStringSet(Constants.SHARED_PREFS_SAVED_EMAIL, null)).thenReturn(savedEmails);

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

You'll have to add true to isLoggedIn then

This comment has been minimized.

@rohanagarwal94

rohanagarwal94 Jun 23, 2017

Contributor

If the user is logged in, why to verify that the adapter of the autocompleteview is set in attachemails method? isloggedin should be false here IMO.

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

Yeah, sorry

@rohanagarwal94

This comment has been minimized.

Copy link
Contributor

rohanagarwal94 commented Jun 23, 2017

@iamareebjamal please check.


loginPresenter.attach(loginView);

Mockito.verifyZeroInteractions(loginView);

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

Make this Mockito.verify(loginView, Mockito.never()).attachEmails();

Because we may want to add other interactions in future which will fail these tests.

Other than that, eveything's fine

This comment has been minimized.

@iamareebjamal

iamareebjamal Jun 23, 2017

Member

Also, remove conflicts as everything else is done

@rohanagarwal94 rohanagarwal94 force-pushed the rohanagarwal94:autocomplete branch from 16ad902 to 066721a Jun 23, 2017

@rohanagarwal94

This comment has been minimized.

Copy link
Contributor

rohanagarwal94 commented Jun 23, 2017

@mejariamol @iamareebjamal please review.

@iamareebjamal

This comment has been minimized.

Copy link
Member

iamareebjamal commented Jun 23, 2017

@rohanagarwal94 Please check the failing tests

@rohanagarwal94 rohanagarwal94 force-pushed the rohanagarwal94:autocomplete branch from 066721a to 94c77fe Jun 23, 2017

@iamareebjamal

This comment has been minimized.

Copy link
Member

iamareebjamal commented Jun 23, 2017

@rohanagarwal94 Not related to this commit, but you might want to update your config email and name on your machine or else your contributions will be shown done by Travis CI, or any anonymous contributor and mentors may get confused that it is not your contribution. Furthermore, look at this for a good commit message guideline https://chris.beams.io/posts/git-commit/

Generally, use imperative present tense, like fix rather than fixes or fixed and there is generally no need to include the issue fixed in the commit title itself, but may be there in the body if you desire to explain the commit itself

@mariobehling mariobehling merged commit fb7d57b into fossasia:master Jun 23, 2017

1 of 2 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rohanagarwal94 rohanagarwal94 deleted the rohanagarwal94:autocomplete branch Jun 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment