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

intentando resolver issue #28 #29

Closed
wants to merge 8 commits into from

Conversation

GonzaloGaleano
Copy link

en el ejemplo se mantiene el último host configurado: 192.168.0.33
además recomiendo utilizar values/strings separados según entorno

… agregado el host en este archivo y en el valor host_name en: app/src/debug/res/values/strings.xml
mencionando los cambios necesarios para apuntar a distintos nombres de servidores según se necesite en desarrollo o producción
Copy link
Member

@alefq alefq left a comment

Choose a reason for hiding this comment

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

Gracias por el aporte @GonzaloGaleano , te dejé algunos comentarios para ciertos cambios necesarios por si esté dentro de tus posibilidades hacerlos.

@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Interesante recurso @GonzaloGaleano , es una buena opción para cierto tipo de constantes.
Solemos usar el build.gradle para definir variables dependientes del buildType.

Copy link
Author

Choose a reason for hiding this comment

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

si, utilizo mucho este recurso también para tener distintos iconos para la versión dev + agregando sufix a appname en build.gradle entonces poder tener debug y release instalados en el mismo teléfono y diferenciados

@@ -1,9 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<network-security-config>
<base-config cleartextTrafficPermitted="false" />
Copy link
Member

Choose a reason for hiding this comment

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

Asegurarse de que una vez resuelto el input text para el hostname, al ingresar un nuevo hostname en runtime; se pueda conectar a un puerto plain text.

Copy link
Author

Choose a reason for hiding this comment

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

En este caso lo resolví con un archivo solo para la versión DEBUG con base-config cleartextTrafficPermitted=true y false para la versión release. Ahí fue el siguiente pull request.

String url = getString(R.string.jwt_URL);
String host_name = getString(R.string.host_name);
String end_point = getString(R.string.jwt_URL);
String url = host_name + end_point;
Copy link
Member

Choose a reason for hiding this comment

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

Sobre variable host_name :

  1. El proyecto tiene su larga data, no obstante nuevas contribuciones tratamos de que mantengan el Coding Style de Android, como se menciona en el README
  2. Su valor debería venir de un input text en la pantalla del login. Esta funcionalidad es la más buscada del ejercicio. El objetivo es que un tester o developer, pueda ingresar un nuevo server en runtime

Copy link
Author

Choose a reason for hiding this comment

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

Si, suelo utilizar camelCase donde se puede, aquí se me pasó. Voy a estar repasando más Android Coding Style.

@GonzaloGaleano
Copy link
Author

Quizás hayan cosas que ajustar, pero en línea general está el prompt que solo se activa en DEBUG y setea una variable en el Singleton que se utilizará en los siguiente lugares que se requieran.
La función que levanta el prompt la hice reutilizable y la dejé en el singleton aunque podría definirse en una ClaseUtil para el mismo.

@GonzaloGaleano GonzaloGaleano requested a review from alefq June 5, 2021 05:34
Copy link
Member

@alefq alefq left a comment

Choose a reason for hiding this comment

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

Gonzalo, está resuelto en general el issue; no obstante dejo algunos comentarios para depurar un poco el código y el flujo en general; en caso de que te interese seguir con la contribución.

@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<network-security-config>
<base-config cleartextTrafficPermitted="true" />
Copy link
Member

Choose a reason for hiding this comment

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

Preferimos que la opción default sea siempre segura. O utilizar un whitelist de host/domains; o que la opción sea forzar al uso de https.


builder.setView(view)
.setPositiveButton(R.string.button_accept, (dialog, id) -> {
if ( inputHostName.getText().toString().equals("") ) {
Copy link
Member

Choose a reason for hiding this comment

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

Por lo general usamos StringUtils o algo que aplique defensive programming para prevenir valores nulos. Por ej. string no nulo

@@ -122,7 +169,7 @@ public static void setSingletonApplicationContext(Context singletonApplicationCo
synchronized public Tracker getDefaultTracker() {
if (mTracker == null) {
this.analytics = GoogleAnalytics.getInstance(this);
analytics.getInstance(this).setLocalDispatchPeriod(1);
GoogleAnalytics.getInstance(this).setLocalDispatchPeriod(1);
Copy link
Member

Choose a reason for hiding this comment

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

¿a qué se debe este cambio? En la línea anterior se está asigando ya a un campo de la clase.

@@ -49,12 +49,14 @@ public void onClick(View v) {
finish();
}
});
MboehaoApp.prompDevHostName(this);
Copy link
Member

Choose a reason for hiding this comment

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

No es necesario preguntar en varias partes; el LoginActivity es el punto de entrada donde se requiere el valor del hostname. En los callbacks del dialog sí está bien como pusiste, aquí ya es redundante.

@@ -34,6 +36,7 @@ protected void onCreate(Bundle savedInstanceState) {
Bundle b = getIntent().getExtras();
assert b != null;
secret = (String) b.get("SECRET");
MboehaoApp.prompDevHostName(this);
Copy link
Member

Choose a reason for hiding this comment

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

No es necesario preguntar en varias partes; el LoginActivity es el punto de entrada donde se requiere el valor del hostname. En los callbacks del dialog sí está bien como pusiste, aquí ya es redundante.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants