SELV3-785: Dashboard reports definition #10
Conversation
|
|
||
| package org.openlmis.report.i18n; | ||
|
|
||
| public class DashboardReportMessageKeys extends MessageKeys { |
There was a problem hiding this comment.
I would like to change this pattern, in my opinion this is super confusing and plain bad.
I propose to type the key text as-is, ex:
public class DashboardReportMessageKeys {
public static final String ERROR_DASHBOARD_REPORT_NAME_DUPLICATED = "report.error.servicedashboardReport.name.duplicated";
public static final String ERROR_DASHBOARD_REPORT_NOT_FOUND = "report.error.servicedashboardReport.notFound";
// ...
}
|
|
||
| package org.openlmis.report.i18n; | ||
|
|
||
| public class ReportCategoryMessageKeys extends ReportingMessageKeys { |
There was a problem hiding this comment.
See DashboardReportMessageKeys comment.
| @@ -74,5 +75,10 @@ public String obtainAccessToken() { | |||
| void setRestTemplate(RestOperations restTemplate) { | |||
| this.restTemplate = restTemplate; | |||
| } | |||
|
|
|||
| @CacheEvict(cacheNames = "token", allEntries = true) | |||
There was a problem hiding this comment.
I don't see this 'token' cache anywhere.
Is org.springframework.cache.annotation.Cacheable missing from obtainAccessToken?
Also, I see this caching not being used in core services - do we know if this is any perf impro we can move later to core services?
There was a problem hiding this comment.
This is something that I took while moving methods from other services. However now I verified and it is not being used in both. For the time being I will remove it.
| import org.springframework.data.repository.PagingAndSortingRepository; | ||
| import org.springframework.data.repository.query.Param; | ||
|
|
||
| public interface DashboardReportRepository |
There was a problem hiding this comment.
Some methods here, have parameters annotated with @param some not, and I can't find a pattern.
| } | ||
| } | ||
|
|
||
| protected <P> void runWithRetryAndTokenRetry(HttpTask<P> task) { |
There was a problem hiding this comment.
I think this method (but in different service) had some bad consequences in Malawi where a call from Requisition to Stock Management would be fired twice (as the 1st would timeout), and added twice as much data to DB, and made us wait twice the time to know it fails.
I think this retry logic (for 4xx and 5xx) might be a bad design.
The token expiration is fine, but do we know why we retry on any 4xx and 5xx?
There was a problem hiding this comment.
You're right. I don't think we need 4xx 5xx retries. I will leave runWithTokenRetry only and change method type to void since the response is not being used.
| } catch (DataIntegrityViolationException ex) { | ||
| throw new ValidationMessageException(new Message( | ||
| DashboardReportMessageKeys.ERROR_DASHBOARD_REPORT_NAME_DUPLICATED, dto.getName()), ex); | ||
| } catch (Exception ex) { |
There was a problem hiding this comment.
I would split this two saves in own try-catch - now 'a random' error from saveDashboardReport would be reported as 'count not save right'.
| public DashboardReportDto createDashboardReport(DashboardReportDto dto) { | ||
| permissionService.canManageReports(); | ||
|
|
||
| boolean reportExists = dashboardReportRepository.findByName(dto.getName()).isPresent(); |
There was a problem hiding this comment.
I suggest to use boolean existsByName() pattern - again not much of performance gain in this scenario, but I think it's just a good practice.
| * @return New RightDto | ||
| */ | ||
| private RightDto createRight(String dashboardReportName) { | ||
| String transformedName = dashboardReportName.trim().toUpperCase().replace(" ", "_") + "_RIGHT"; |
There was a problem hiding this comment.
Does a right name must be a valid file.properties key? If so, I would made small class with something like transformToPropKey - maybe there is already such lib - because there are more illegal characters then just a 'space'.
There was a problem hiding this comment.
Thanks. Good observation.
| public ReportCategoryDto createReportCategory(ReportCategoryDto dto) { | ||
| permissionService.canManageReportCategories(); | ||
|
|
||
| reportCategoryRepository.findByName(dto.getName()) |
There was a problem hiding this comment.
I suggest reportCategoryRepository.existsByName(dto.getName()))
| .orElseThrow(() -> new NotFoundMessageException( | ||
| ReportCategoryMessageKeys.ERROR_REPORT_CATEGORY_NOT_FOUND)); | ||
|
|
||
| reportCategoryRepository.findByName(dto.getName()) |
There was a problem hiding this comment.
I think this might be solved by single Spring Data method: boolean existsByIdIsNotAndName(id, name); or similar.
|
Added dashboard reports definition:
Endpoints:
/api/reports/dashboardReports
/api/reports/reportCategories