Skip to content

Conversation

@ak2502
Copy link
Contributor

@ak2502 ak2502 commented Aug 22, 2023

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

@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 <akanksha.feb25@gmail.com>
…Repository (removed it as a parameter from function)

Signed-off-by: ak2502 <akanksha.feb25@gmail.com>
Signed-off-by: ak2502 <akanksha.feb25@gmail.com>
Signed-off-by: ak2502 <akanksha.feb25@gmail.com>
@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

Development

Successfully merging this pull request may close these issues.

Change the 5 graphs to start using the new endpoints Add endpoints to retrieve data for 5 graphs on the HR tab

2 participants