You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2021/12/04 10:46:55 UTC
[GitHub] [fineract] ptuomola opened a new pull request #1994: Implement integration tests for twofactor profile
ptuomola opened a new pull request #1994:
URL: https://github.com/apache/fineract/pull/1994
## Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766832235
##########
File path: fineract-provider/src/main/resources/sql/migrations/core_db/V265__modify_external_service_schema.sql
##########
@@ -53,9 +53,9 @@ insert into c_external_service_properties (`name`, `value`, `external_service_id
insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('password', 'support80', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'smtp.gmail.com', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'localhost', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '587', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '3025', (select id from c_external_service where name = 'SMTP_Email_Account'));
Review comment:
Traditionally we tried to avoid changing existing migration files (because then the DB after a migration and a fresh install is not the same anymore) so perhaps you would consider putting this into a new file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766830185
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/service/ExternalServicesPropertiesReadPlatformServiceImpl.java
##########
@@ -83,7 +83,7 @@ public SMTPCredentialsData extractData(final ResultSet rs) throws SQLException,
if (ExternalServicesConstants.SMTP_USERNAME.equalsIgnoreCase(name)) {
username = value;
- } else if (ExternalServicesConstants.SMTP_PORT.equalsIgnoreCase(port)) {
+ } else if (ExternalServicesConstants.SMTP_PORT.equalsIgnoreCase(name)) {
Review comment:
@ptuomola this change looks curious - is this intentional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#issuecomment-991133978
Looks like this implements https://issues.apache.org/jira/browse/FINERACT-1335! NICE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766943898
##########
File path: twofactor-tests/src/test/java/org/apache/fineract/twofactortests/common/Utils.java
##########
@@ -0,0 +1,267 @@
+/**
Review comment:
I'm only using two methods from that so can inline them into the other class and delete this.
I agree that it would be good to move away from restassured etc. But I haven't spent much time with fineract-client yet, and just wanted to quickly come up with something that stops us from accidentally breaking non-basicauth profiles - as we had recently done without noticing. Very happy if someone wants to do this better ie with fineract-client etc...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r767209408
##########
File path: twofactor-tests/src/test/java/org/apache/fineract/twofactortests/common/Utils.java
##########
@@ -0,0 +1,267 @@
+/**
Review comment:
SGTM. Do you want to do that (inline) and I'll merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766835893
##########
File path: twofactor-tests/build.gradle
##########
@@ -0,0 +1,67 @@
+/**
Review comment:
copy/paste of https://github.com/apache/fineract/blob/develop/integration-tests/build.gradle ... I don't want to be a PITA, but obviously have to ask if there could be a nice way with a bit of Gradle foo to avoid this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #1994:
URL: https://github.com/apache/fineract/pull/1994
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r767209707
##########
File path: fineract-provider/src/main/resources/sql/migrations/core_db/V265__modify_external_service_schema.sql
##########
@@ -53,9 +53,9 @@ insert into c_external_service_properties (`name`, `value`, `external_service_id
insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('password', 'support80', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'smtp.gmail.com', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'localhost', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '587', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '3025', (select id from c_external_service where name = 'SMTP_Email_Account'));
Review comment:
SGTM, then we'll do this as an "exception to the rule".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766837852
##########
File path: twofactor-tests/src/test/java/org/apache/fineract/twofactortests/common/Utils.java
##########
@@ -0,0 +1,267 @@
+/**
Review comment:
Copy/paste of https://github.com/apache/fineract/blob/develop/integration-tests/src/test/java/org/apache/fineract/integrationtests/common/Utils.java ... hm. Too much of a PITA to centralized this into a common integration-test-lib kinda module, maybe? Or... just not write the `TwoFactorAuthenticationTest` with this but e.g. using the `fineract-client`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766831540
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/service/ExternalServicesPropertiesReadPlatformServiceImpl.java
##########
@@ -83,7 +83,7 @@ public SMTPCredentialsData extractData(final ResultSet rs) throws SQLException,
if (ExternalServicesConstants.SMTP_USERNAME.equalsIgnoreCase(name)) {
username = value;
- } else if (ExternalServicesConstants.SMTP_PORT.equalsIgnoreCase(port)) {
+ } else if (ExternalServicesConstants.SMTP_PORT.equalsIgnoreCase(name)) {
Review comment:
sorry yeah no this is obviously right (it makes more sense looking at the full file then the diff here), nice catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766938895
##########
File path: fineract-provider/src/main/resources/sql/migrations/core_db/V265__modify_external_service_schema.sql
##########
@@ -53,9 +53,9 @@ insert into c_external_service_properties (`name`, `value`, `external_service_id
insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('password', 'support80', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'smtp.gmail.com', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'localhost', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '587', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '3025', (select id from c_external_service where name = 'SMTP_Email_Account'));
Review comment:
The reason why I chose this "unorthodox" approach is that we don't want to overwrite people's existing SMTP configurations. We only want to change the default for new systems that is used in testing. So we don't want to migrate/change any existing setups (as adding a new migration script would do), we want to just change the value that is used when setting up a new system...
Another approach would be to have some custom SQL overwriting the initial values in testing. But as no one should be using the initial values anyway in their systems, why not just change them...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766938895
##########
File path: fineract-provider/src/main/resources/sql/migrations/core_db/V265__modify_external_service_schema.sql
##########
@@ -53,9 +53,9 @@ insert into c_external_service_properties (`name`, `value`, `external_service_id
insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('password', 'support80', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'smtp.gmail.com', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'localhost', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '587', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '3025', (select id from c_external_service where name = 'SMTP_Email_Account'));
Review comment:
The reason why I chose this "unorthodox" approach is that we don't want to overwrite people's existing SMTP configurations. We only want to change the default for new systems that is used in testing. So we don't want to migrate/change any existing setups (as adding a new migration script would do), we want to just change the value that is used when setting up a new system...
Another approach would be to have some custom SQL overwriting the initial values in testing. But as no one should be using the initial values anyway in their systems, why not just change them...
##########
File path: twofactor-tests/build.gradle
##########
@@ -0,0 +1,67 @@
+/**
Review comment:
I tried to initially do something along those lines i.e. have one master cargo task and then inherit from that. I couldn't get it to work so thought it's easier just to have two parallel projects with no dependencies between them. But I'm not very good with Gradle so very happy if someone can do something more elegant...
##########
File path: twofactor-tests/src/test/java/org/apache/fineract/twofactortests/common/Utils.java
##########
@@ -0,0 +1,267 @@
+/**
Review comment:
I'm only using two methods from that so can inline them into the other class and delete this.
I agree that it would be good to move away from restassured etc. But I haven't spent much time with fineract-client yet, and just wanted to quickly come up with something that stops us from accidentally breaking non-basicauth profiles - as we had recently done without noticing. Very happy if someone wants to do this better ie with fineract-client etc...
##########
File path: twofactor-tests/src/test/java/org/apache/fineract/twofactortests/common/Utils.java
##########
@@ -0,0 +1,267 @@
+/**
Review comment:
Done... took me a bit longer than I expected, as I found a time rounding error in the code as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r767221783
##########
File path: twofactor-tests/src/test/java/org/apache/fineract/twofactortests/common/Utils.java
##########
@@ -0,0 +1,267 @@
+/**
Review comment:
Done... took me a bit longer than I expected, as I found a time rounding error in the code as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r766941025
##########
File path: twofactor-tests/build.gradle
##########
@@ -0,0 +1,67 @@
+/**
Review comment:
I tried to initially do something along those lines i.e. have one master cargo task and then inherit from that. I couldn't get it to work so thought it's easier just to have two parallel projects with no dependencies between them. But I'm not very good with Gradle so very happy if someone can do something more elegant...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r767209605
##########
File path: twofactor-tests/build.gradle
##########
@@ -0,0 +1,67 @@
+/**
Review comment:
SGTM. Maybe this is a clean up that e.g. @vidakovic who has great Gradle foo ;) wants to do as a follow up after this is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1994: Implement integration tests for twofactor profile
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1994:
URL: https://github.com/apache/fineract/pull/1994#discussion_r767209408
##########
File path: twofactor-tests/src/test/java/org/apache/fineract/twofactortests/common/Utils.java
##########
@@ -0,0 +1,267 @@
+/**
Review comment:
SGTM. Do you want to do that (inline) and I'll merge this?
##########
File path: twofactor-tests/build.gradle
##########
@@ -0,0 +1,67 @@
+/**
Review comment:
SGTM. Maybe this is a clean up that e.g. @vidakovic who has great Gradle foo ;) wants to do as a follow up after this is merged.
##########
File path: fineract-provider/src/main/resources/sql/migrations/core_db/V265__modify_external_service_schema.sql
##########
@@ -53,9 +53,9 @@ insert into c_external_service_properties (`name`, `value`, `external_service_id
insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('password', 'support80', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'smtp.gmail.com', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('host', 'localhost', (select id from c_external_service where name = 'SMTP_Email_Account'));
-insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '587', (select id from c_external_service where name = 'SMTP_Email_Account'));
+insert into c_external_service_properties (`name`, `value`, `external_service_id`) values('port', '3025', (select id from c_external_service where name = 'SMTP_Email_Account'));
Review comment:
SGTM, then we'll do this as an "exception to the rule".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org