avniproject / avni-etl

GNU Affero General Public License v3.0
0 stars 6 forks source link

Added HR tab for canned reports using ETL #68

Open ak2502 opened 1 year ago

ak2502 commented 1 year ago

AvinashRamachandruni/avni-etl#3 AvinashRamachandruni/avni-etl#6 AvinashRamachandruni/avni-etl#14 AvinashRamachandruni/avni-etl#15 avniproject/avni-product#1334

vinayvenu commented 1 year ago

@ak2502 @AvinashRamachandruni Some comments below

General

ReportRepository

ReportController

General comments

getSummaryTable

getUserActivity

StrUtil

ak2502 commented 1 year ago

@vinayvenu, Made the changes and required clarifcations

General

* Unit/integration tests missing --couldn't find any report related tests in avni-server. What needs to be added here?

ReportRepository

* In all methods, the schemaName need not be passed in from outside. This can be identified within the repository --fixed

* All queries can move to .sql or .sql.st files instead of being in the repository --query for generateUserActivity includes inputs from schemaMetadata. so currently it will take some time to figure out how I can incorporate it into sql file

* Add a logger.debug before running sql queries so that we can figure out the sql if required

* Use runInOrgContext for all queries so we do not send out information for a different organisation even by mistake --fixed

ReportController

General comments

* getDynamicWhere is not a controller level directive. Any sql level changes should stay within the repository

* getDynamicWhere is dangerous code that can cause sql injection attacks. All parameters need to be sanitised --does this mean i need to add privilege type as it is there in avni-server? If not, how to fix this?

* What is the purpose of two calls - getUserWiseDeviceModels and getUserWiseAppVersions? Can't this have been a single call with a url of say, /users/stats? --these represent 2 different pie charts involving different sql

* It will be useful to have a default start and end dates if not specified

* Use specific classes for responses instead of a loose AggregateReportResult class --this is being used for all pie charts in a similar way in avni-server 

* I don't see a purpose of adding a list of userIds in the request. Is it useful? --it was being used in avni-server but is not currently used here --removed and fixed

getSummaryTable

* Does not use any of its parameters --removed the parameters and fixed

getUserActivity

* If there are 100 users and 100 days and 20 tables, the result will have 2 lakh rows. This is unacceptable. There needs to be a limit somewhere --there is a limit of only top 10 users

StrUtil

* Use full names (StringUtils). --fixed

* isEmpty is already in org.springframework.util.StringUtils.hasLength() --removed and fixed
vinayvenu commented 1 year ago

Temporarily merged to canned-analytics branch