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

Added HR tab for canned reports using ETL #68

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

ak2502
Copy link
Contributor

@ak2502 ak2502 commented Aug 22, 2023

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

Avinash Ramachandruni and others added 23 commits August 1, 2023 07:46
…d userSyncStatus endpoint and verified working

Signed-off-by: ak2502 <[email protected]>
…hema - deviceModels, appVersions, userDetails
…eByProportion query according to ETL

Signed-off-by: ak2502 <[email protected]>
…show weekly data for past 3 months

Signed-off-by: ak2502 <[email protected]>
@vinayvenu
Copy link
Member

@ak2502 @AvinashRamachandruni Some comments below

General

  • Unit/integration tests missing

ReportRepository

  • In all methods, the schemaName need not be passed in from outside. This can be identified within the repository
  • All queries can move to .sql or .sql.st files instead of being in the repository
  • 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

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
  • 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?
  • 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
  • I don't see a purpose of adding a list of userIds in the request. Is it useful?

getSummaryTable

  • Does not use any of its parameters

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

StrUtil

  • Use full names (StringUtils).
  • isEmpty is already in org.springframework.util.StringUtils.hasLength()

1) added runInOrgContext in ReportRepository
2) removed getSummaryTable paramenters in ReportController
3) renamed StrUtil to StringUtil and removed isEmpty class as it is a predefined class

Signed-off-by: ak2502 <[email protected]>
…Repository (removed it as a parameter from function)

Signed-off-by: ak2502 <[email protected]>
@ak2502
Copy link
Contributor Author

ak2502 commented Sep 16, 2023

@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
Copy link
Member

Temporarily merged to canned-analytics branch

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