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 2022/07/25 08:34:52 UTC
[GitHub] [fineract] galovics commented on a diff in pull request #2446: Datatables datetime fields management
galovics commented on code in PR #2446:
URL: https://github.com/apache/fineract/pull/2446#discussion_r928607152
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1492,6 +1494,48 @@ private List<ResultsetRowData> fillDatatableResultSetDataRows(final String sql)
return resultsetDataRows;
}
+ private List<ResultsetRowData> fillDatatableResultSetDataRows(final String sql, final List<ResultsetColumnHeaderData> columnHeaders) {
+ final List<ResultsetRowData> resultsetDataRows = new ArrayList<>();
+ final GenericResultsetData genericResultsetData = new GenericResultsetData(columnHeaders, null);
+
+ final SqlRowSet rowSet = jdbcTemplate.queryForRowSet(sql); // NOSONAR
+ final SqlRowSetMetaData rsmd = rowSet.getMetaData();
+
+ while (rowSet.next()) {
+ final List<Object> columnValues = new ArrayList<>();
+
+ for (int i = 0; i < rsmd.getColumnCount(); i++) {
+ final String columnName = rsmd.getColumnName(i + 1);
+ final String colType = genericResultsetData.getColTypeOfColumnNamed(columnName);
+ if ("DECIMAL".equalsIgnoreCase(colType)) {
+ columnValues.add(rowSet.getBigDecimal(columnName));
+ } else if ("DATE".equalsIgnoreCase(colType)) {
+ columnValues.add(rowSet.getDate(columnName));
+ } else if ("timestamp without time zone".equalsIgnoreCase(colType) // PostgreSQL
+ || "datetime".equalsIgnoreCase(colType)) { // MariaDB
+ Date dateVal = null;
+ if (databaseTypeResolver.isMySQL()) {
+ try {
+ columnValues.add(rowSet.getObject(columnName));
+ } catch (InvalidResultSetAccessException e) {
+ LOG.error("ERROR: {}", e.getMessage());
Review Comment:
Bad pattern, let's just use the Logger#error overload which accepts an exception in its own, no need to only log the message.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/GenericDataServiceImpl.java:
##########
@@ -160,23 +161,32 @@ public String generateJsonFromGenericResultsetData(final GenericResultsetData gr
if (currColType == null && colType.equalsIgnoreCase("DATE")) {
currColType = "DATE";
}
+ if (currColType == null && colType.equalsIgnoreCase("DATETIME")) {
+ currColType = "DATETIME";
+ }
currVal = row.get(j);
if (currVal != null && currColType != null) {
if (currColType.equals("DECIMAL") || currColType.equals("INTEGER")) {
writer.append(currVal);
} else {
if (currColType.equals("DATE")) {
- final LocalDate localDate = LocalDate.parse(currVal);
+ final LocalDate localDate = LocalDate.parse(currVal.toString());
writer.append(
"[" + localDate.getYear() + ", " + localDate.getMonthValue() + ", " + localDate.getDayOfMonth() + "]");
} else if (currColType.equals("DATETIME")) {
- final LocalDateTime localDateTime = LocalDateTime.parse(formatDateTimeValue(currVal),
- DateUtils.DEFAULT_DATETIME_FORMATER);
- writer.append("[" + localDateTime.getYear() + ", " + localDateTime.getMonthValue() + ", "
- + localDateTime.getDayOfMonth() + ", " + localDateTime.getHour() + ", " + localDateTime.getMinute()
- + ", " + localDateTime.getSecond() + ", " + localDateTime.get(ChronoField.MILLI_OF_SECOND) + "]");
+ Calendar dateVal = Calendar.getInstance();
Review Comment:
Why are we using an old Calendar object here?
Can't we just use a LocalDateTime instead?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1492,6 +1494,48 @@ private List<ResultsetRowData> fillDatatableResultSetDataRows(final String sql)
return resultsetDataRows;
}
+ private List<ResultsetRowData> fillDatatableResultSetDataRows(final String sql, final List<ResultsetColumnHeaderData> columnHeaders) {
+ final List<ResultsetRowData> resultsetDataRows = new ArrayList<>();
+ final GenericResultsetData genericResultsetData = new GenericResultsetData(columnHeaders, null);
+
+ final SqlRowSet rowSet = jdbcTemplate.queryForRowSet(sql); // NOSONAR
+ final SqlRowSetMetaData rsmd = rowSet.getMetaData();
Review Comment:
Isn't this whole logic pretty much a duplicate of the fillDatatableResultSetDataRows#String version? We should definitely extract whatever is common instead of duping it completely.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/GenericDataServiceImpl.java:
##########
@@ -160,23 +161,32 @@ public String generateJsonFromGenericResultsetData(final GenericResultsetData gr
if (currColType == null && colType.equalsIgnoreCase("DATE")) {
currColType = "DATE";
}
+ if (currColType == null && colType.equalsIgnoreCase("DATETIME")) {
+ currColType = "DATETIME";
+ }
currVal = row.get(j);
if (currVal != null && currColType != null) {
if (currColType.equals("DECIMAL") || currColType.equals("INTEGER")) {
writer.append(currVal);
} else {
if (currColType.equals("DATE")) {
- final LocalDate localDate = LocalDate.parse(currVal);
+ final LocalDate localDate = LocalDate.parse(currVal.toString());
writer.append(
"[" + localDate.getYear() + ", " + localDate.getMonthValue() + ", " + localDate.getDayOfMonth() + "]");
} else if (currColType.equals("DATETIME")) {
- final LocalDateTime localDateTime = LocalDateTime.parse(formatDateTimeValue(currVal),
- DateUtils.DEFAULT_DATETIME_FORMATER);
- writer.append("[" + localDateTime.getYear() + ", " + localDateTime.getMonthValue() + ", "
- + localDateTime.getDayOfMonth() + ", " + localDateTime.getHour() + ", " + localDateTime.getMinute()
- + ", " + localDateTime.getSecond() + ", " + localDateTime.get(ChronoField.MILLI_OF_SECOND) + "]");
+ Calendar dateVal = Calendar.getInstance();
+ if (currVal instanceof Date) {
+ dateVal.setTime((Date) currVal);
+ } else if (currVal instanceof LocalDateTime) {
+ dateVal.setTime(
+ Date.from(((LocalDateTime) currVal).atZone(DateUtils.getDateTimeZoneOfTenant()).toInstant()));
Review Comment:
You sure about this cause I'm not. Are we ensuring that we're saving the datetime value into the datatable with the tenant's timezone?
I don't think so. Can you please double-check?
cc @adamsaghy
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/GenericDataServiceImpl.java:
##########
@@ -160,23 +161,32 @@ public String generateJsonFromGenericResultsetData(final GenericResultsetData gr
if (currColType == null && colType.equalsIgnoreCase("DATE")) {
currColType = "DATE";
}
+ if (currColType == null && colType.equalsIgnoreCase("DATETIME")) {
+ currColType = "DATETIME";
+ }
currVal = row.get(j);
if (currVal != null && currColType != null) {
if (currColType.equals("DECIMAL") || currColType.equals("INTEGER")) {
writer.append(currVal);
} else {
if (currColType.equals("DATE")) {
- final LocalDate localDate = LocalDate.parse(currVal);
+ final LocalDate localDate = LocalDate.parse(currVal.toString());
writer.append(
"[" + localDate.getYear() + ", " + localDate.getMonthValue() + ", " + localDate.getDayOfMonth() + "]");
} else if (currColType.equals("DATETIME")) {
- final LocalDateTime localDateTime = LocalDateTime.parse(formatDateTimeValue(currVal),
- DateUtils.DEFAULT_DATETIME_FORMATER);
- writer.append("[" + localDateTime.getYear() + ", " + localDateTime.getMonthValue() + ", "
- + localDateTime.getDayOfMonth() + ", " + localDateTime.getHour() + ", " + localDateTime.getMinute()
- + ", " + localDateTime.getSecond() + ", " + localDateTime.get(ChronoField.MILLI_OF_SECOND) + "]");
+ Calendar dateVal = Calendar.getInstance();
+ if (currVal instanceof Date) {
+ dateVal.setTime((Date) currVal);
+ } else if (currVal instanceof LocalDateTime) {
+ dateVal.setTime(
+ Date.from(((LocalDateTime) currVal).atZone(DateUtils.getDateTimeZoneOfTenant()).toInstant()));
+ }
+ writer.append("[" + dateVal.get(Calendar.YEAR) + ", " + (dateVal.get(Calendar.MONTH) + 1) + ", "
Review Comment:
This whole thing could be replaced with a String#format call.
##########
fineract-provider/src/main/resources/db/changelog/tenant/parts/0019_refactor_loan_transaction.xml:
##########
@@ -23,6 +23,7 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.1.xsd">
<changeSet author="fineract" id="1" onValidationFail="MARK_RAN">
+ <validCheckSum>8:177f2e2cbac12752ede2182bbbfde6ae</validCheckSum>
Review Comment:
You sure this is still needed? I think it's fixed on develop already.
--
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