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

Developed Login for DevAuthBackEnd #48 #65

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kunall17
Collaborator

kunall17 commented Jun 1, 2016

Now we can directly login using the devAuthBackEnd using a developement server! (#48)

Has a menu same as the Web UI

If the devAuthBackEnd is not enabled then it shows a toast error message like this

Server code here

@smarx

This comment has been minimized.

smarx commented Jun 1, 2016

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jun 1, 2016

hey @kunall17 this looks really cool.

import java.util.List;
public class DevAuthActivity extends Activity {
RecyclerView recyclerView;

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

this should be private

super.onCreate(savedInstanceState);
setContentView(R.layout.activity_dev_auth);
recyclerView = (RecyclerView) findViewById(R.id.devAuthRecyclerView);
String json = getIntent().getStringExtra("emails_json");

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

might be nice to move emails_json to a constant somewhere that can be referenced by all uses across the app.

emails.add(jsonArray.get(i).toString());
}
} catch (JSONException e) {
e.printStackTrace();

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

please don't use e.printStackTrace(). either use ZLog or Log. (preference for ZLog).

private static final int VIEW_TYPE_ADMIN = 1;
private static final int VIEW_TYPE_USER = 2;
List<String> emails;
int direct_admin_size;

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

this style of variable naming is not great. please use camel casing. ie directAdminSize.

else ((TextView) holder.view).setText(R.string.admin);
} else if (position == direct_admin_size + 1)
((TextView) holder.view).setText(R.string.normal_user);
else if (position > direct_admin_size + 1) {

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

this + 1 / - 2 sort of stuff indexing is pretty brittle. is there a better way to do this?

@Override
public int getItemCount() {
return emails.size() + 2;

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

why are you adding 2 here? can you add a comment about it?

This comment has been minimized.

@kunall17

kunall17 Jun 2, 2016

Collaborator

+2 due to the two headers {"Admin and user"}, added comment as well.

super(itemView);
view = itemView.findViewById(android.R.id.text1);
if (view instanceof Button) {
((Button) view).setOnClickListener(new View.OnClickListener() {

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

i'm not sure you need the Button cast & check to add a setOnClickListener (I think all views have this method).

@@ -233,15 +233,17 @@ public void onClick(View v) {
case R.id.google_sign_in_button:
connectionProgressDialog.show();
saveServerURL();
Toast.makeText(this, getString(R.string.logging_into_server, ZulipApp.get().getServerURI()), Toast.LENGTH_SHORT).show();

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

this message is for sign ins that are to servers other than Zulip. here, it'll show for G+ logins, which is incorrect. (right now you can't log in to G+ for a server other than Zulip.com)

This comment has been minimized.

@kunall17

kunall17 Jun 2, 2016

Collaborator

It was there since before so I added that line as it is!
Should I remove that, it seems correct though as we are logging in to a server and we should give them a message that goes like (Logging into server :{server url}) ?

This comment has been minimized.

@niftynei

niftynei Jun 4, 2016

Contributor

i'm ok with leaving it :)

default:
break;
}
}
private boolean isInputValid() {
private boolean isInputValid(boolean checkEmailAndPassword) {

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

this is sort of hacky. is there a better way to do this?

This comment has been minimized.

@kunall17

kunall17 Jun 2, 2016

Collaborator

I'll have to create another method or just use this code directly without method ?

This comment has been minimized.

@niftynei

niftynei Jun 2, 2016

Contributor

i'm in favor of there being a separate method or path for dev-only options.

import org.json.JSONObject;
/**
* Created by kunall17 on 5/31/16.

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

Can you get rid of this comment? Zulip projects rely on commit history to document authorship.

context.startActivity(intent);
}
} catch (JSONException e) {
e.printStackTrace();

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

see other comment about not using e.printStackTrace()

JSONObject obj = new JSONObject(e.getMessage());
message = obj.getString("msg");
} catch (JSONException e1) {
ZLog.logException(e1);

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

😄

@Override
protected void handleHTTPError(final HttpResponseException e) {
if (e.getStatusCode() == HttpStatus.SC_FORBIDDEN) {
String message = "Network Error.";

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

Any user visible text belongs in R.strings.

@@ -94,10 +94,18 @@
</com.google.android.gms.common.SignInButton>
<TextView
android:id="@+id/local_server_button"

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

is there a way to toggle this for non-dev builds? right now it seems that this will appear on production builds (which is a no go).

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jun 1, 2016

Looks really nice.

Needs a way to not show the dev-server login for production builds.

@@ -46,6 +46,7 @@ android {
dependencies {
compile 'com.android.support:support-v13:23.3.0'
compile 'com.android.support:appcompat-v7:23.3.0'
compile "com.android.support:recyclerview-v7:23.0.1"

This comment has been minimized.

@niftynei

niftynei Jun 1, 2016

Contributor

this is a small thing, but can you remove the period from this commit message?

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jun 2, 2016

Updated :)

@niftynei for hidding in production builds I could hide the textView if BuildConfig.DEBUG is not true?

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jun 2, 2016

totally. BuildConfig.DEBUG should work great 👍

finish();
}
public class AuthEmailAdapter extends RecyclerView.Adapter<DevAuthActivity.AuthEmailViewHolder> {

This comment has been minimized.

@niftynei

niftynei Jun 4, 2016

Contributor

Can you try breaking this object out into a separate class?

This comment has been minimized.

@kunall17

kunall17 Jun 4, 2016

Collaborator

You referring to create another class of asyncLogin for fetching devEmails?
Or talking for the Adapter?

This comment has been minimized.

@niftynei

niftynei Jun 6, 2016

Contributor

For the adapter!

public AsyncLogin(LoginActivity loginActivity, String username, String password) {
super((ZulipApp) loginActivity.getApplication());
context = loginActivity;
public AsyncLogin(Context context, String username, String password, boolean devServer) {

This comment has been minimized.

@niftynei

niftynei Jun 4, 2016

Contributor

Is there a reason that this isn't of type Activity? Below you're casting it to Activity without checking.

This comment has been minimized.

@kunall17

kunall17 Jun 5, 2016

Collaborator

Yeah, didn't notice this thing .. Updated now. 👍

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jun 19, 2016

@niftynei Updated with the Tests And created a separate file for the Adapter.
I have not included the Gradle libraries for the Espresso Test as they are in the #45, should I add that commit here as well?

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jun 22, 2016

@kunall17 i'm having trouble building this branch. it seems like the ClickListener class is missing. here's the compilation error i'm getting:

/Users/dev/zulip-android/app/src/main/java/com/zulip/android/activities/AuthEmailAdapter.java:14: error: cannot find symbol
import com.zulip.android.util.ClickListener;
                             ^
  symbol:   class ClickListener
  location: package com.zulip.android.util
/Users/dev/zulip-android/app/src/main/java/com/zulip/android/activities/AuthEmailAdapter.java:25: error: cannot find symbol
    private static ClickListener clickListener;
@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jun 22, 2016

@niftynei Ohh, so sorry about this missing file fixed this one
And changed to new API get mail route as well.

And yeah this interferes with the iago@zulip.com hack as it sends the default saved (10.0.2.2:9991/api/) URL, should i remove the iago hack in a commit?

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jun 23, 2016

@kunall17 I'm +1 to removing the iago hack :)

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jun 24, 2016

@niftynei Iago hack removed.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 7, 2016

@kunall17 this works great!

I'm going to go ahead and merge this branch in since all I have are requests for improvements. There's 2 things (I'll make an issue for them as well)

  1. There's no spinner when you click on the "Dev Backend testing server" button to let you know that it's talking to the server.
  2. If there's any errors that occur after clicking "Dev Backend testing server", they aren't shown
@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 7, 2016

This has been merged.

@niftynei niftynei closed this Jul 7, 2016

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