OBPIH-5840 Create a Dashboard indicator for number of requests waiting for approval #4424

drodzewicz · 2023-12-18T11:20:47Z

No description provided.

drodzewicz · 2023-12-18T11:21:58Z

Pushing the first part of this ticket which can be reviewed, but waiting for feedback on questions left in the ticket comments.

grails-app/controllers/org/pih/warehouse/api/DashboardApiController.groovy

alannadolny · 2023-12-19T09:27:04Z

grails-app/services/org/pih/warehouse/dashboard/NumberDataService.groovy

+ def openStockRequests = Requisition.executeQuery("""
+ SELECT COUNT(distinct r.id) FROM Requisition r
+ WHERE r.origin = :location
+ AND r.sourceType = :sourceType
+ AND r.status = :status
+ """,
+ [
+ 'location': location,
+ 'sourceType' : RequisitionSourceType.ELECTRONIC,
+ 'status' : RequisitionStatus.PENDING_APPROVAL,
+ ])


Wouldn't it be better to use createCritera here?

I would go even further and give the data service method a shot (as I can see we already have the RequisitionDataService, so this would require implementing only a method)

int countByOriginAndSourceTypeAndStatus(Location origin, RequisitionSourceType sourceType, RequisitionStatus status)

Previously I was just following the example of other NumberService methods which compute data with pure SQL statements, but you are right something like a createCriteria() looks a bit more friendly so I went with that approach

I also tested your suggestion @kchelstowski with countByOriginAndSourceTypeAndStatus() but I realized that we need to cache this indicator and the keys are location.id and currentUser.id so either way i would need to call this RequisitionDataService method in a NumberDataService so it creates a little more clutter and methods that we have to maintain so I decided to go with createCriteria()

Although now that I think about it, I remember you mentioning something that GORM Data Service methods are much faster than the standard criteria ones, so this might be a reason to use the DataService instead?

@drodzewicz could you elaborate a bit on the fact that you couldn't use the data service method here? I don't mind the createCriteria() approach, but wanted to understand why the data service method didn't fit there?

Although now that I think about it, I remember you mentioning something that GORM Data Service methods are much faster than the standard criteria ones, so this might be a reason to use the DataService instead?

I think I've never said they are faster - it should probably evaluate to the same SQL query as the data service method, but imho they are just more readable and they don't require that much of "boilerplate" code - e.g. the createCriteria approach here took more lines than one-liner data service method.

alannadolny · 2023-12-19T09:31:46Z

grails-app/services/org/pih/warehouse/dashboard/NumberDataService.groovy

+ String redirectLink = currentUser.hasRoles(location, [RoleType.ROLE_REQUISITION_APPROVER])
+ ? "${urlContextPath}/stockMovement/list?direction=OUTBOUND&requisitionStatusCode=PENDING_APPROVAL&sourceType=ELECTRONIC"
+ : "${urlContextPath}/stockMovement/list?direction=OUTBOUND&sourceType=ELECTRONIC"


It looks odd to me, but I don't have any idea how to improve that, maybe someone else can come up with a better solution?

I agree, imho we should also think about creating a url factory (similar to the frontend one) as a tech debt ticket.
In here, at the very least, I'd suggest using consts for the param values:

?direction=StockMovementDirection.OUTBOUND&requisitionStatusCode=RequisitionStatus.PENDING_APPROVAL&sourceType=RequisitionSourceType.ELECTRONIC

instead I decided to use the createLink(...) method which is provided by ApplicationTagLib.
That way we don;t have manually prepend the contextPath and we can use the enums and other values in the parameters which later builds the propper string link just like we want to.

kchelstowski · 2023-12-19T10:12:56Z

grails-app/services/org/pih/warehouse/dashboard/NumberDataService.groovy

+ def openStockRequests = Requisition.executeQuery("""
+ SELECT COUNT(distinct r.id) FROM Requisition r
+ WHERE r.origin = :location
+ AND r.sourceType = :sourceType
+ AND r.status = :status
+ """,
+ [
+ 'location': location,
+ 'sourceType' : RequisitionSourceType.ELECTRONIC,
+ 'status' : RequisitionStatus.PENDING_APPROVAL,
+ ])


I would go even further and give the data service method a shot (as I can see we already have the RequisitionDataService, so this would require implementing only a method)

int countByOriginAndSourceTypeAndStatus(Location origin, RequisitionSourceType sourceType, RequisitionStatus status)

kchelstowski · 2023-12-19T10:15:37Z

grails-app/services/org/pih/warehouse/dashboard/NumberDataService.groovy

+ String redirectLink = currentUser.hasRoles(location, [RoleType.ROLE_REQUISITION_APPROVER])
+ ? "${urlContextPath}/stockMovement/list?direction=OUTBOUND&requisitionStatusCode=PENDING_APPROVAL&sourceType=ELECTRONIC"
+ : "${urlContextPath}/stockMovement/list?direction=OUTBOUND&sourceType=ELECTRONIC"


I agree, imho we should also think about creating a url factory (similar to the frontend one) as a tech debt ticket.
In here, at the very least, I'd suggest using consts for the param values:

?direction=StockMovementDirection.OUTBOUND&requisitionStatusCode=RequisitionStatus.PENDING_APPROVAL&sourceType=RequisitionSourceType.ELECTRONIC

…g for approval

…ia intead of pure SQL and build redirect URLs with createLink

…g for approval (#4424) * OBPIH-5840 Create a Dashboard indicator for number of requests waiting for approval * OBPIH-5840 Fix cachablity of indicator when switching users * OBPIH-5840 refactor requestPendingApproval method to use createCriteria intead of pure SQL and build redirect URLs with createLink

drodzewicz added the warn: do not merge Marks a pull request that is not yet ready to be merged label Dec 18, 2023

drodzewicz self-assigned this Dec 18, 2023

drodzewicz added the status: work in progress For issues or pull requests that are in progress but not yet completed label Dec 18, 2023

drodzewicz force-pushed the OBPIH-5840-dashboard-indicator-for-requests-waiting-for-approval-approvals-lists-pages branch from 163e9d6 to ec095d6 Compare December 18, 2023 14:47

drodzewicz changed the base branch from release/0.8.23-hotfix2 to feature/upgrade-to-grails-3.3.10 December 18, 2023 14:47

drodzewicz removed the status: work in progress For issues or pull requests that are in progress but not yet completed label Dec 18, 2023

drodzewicz marked this pull request as ready for review December 18, 2023 15:23

drodzewicz requested review from awalkowiak, kchelstowski, jmiranda and alannadolny December 18, 2023 15:23

drodzewicz changed the title ~~OBPIH-5840 Create a Dashboard indicator for numbe rof requests waiting for approval~~ OBPIH-5840 Create a Dashboard indicator for number of requests waiting for approval Dec 18, 2023

drodzewicz added this to the 0.9.1 milestone Dec 19, 2023

alannadolny reviewed Dec 19, 2023

View reviewed changes

kchelstowski requested changes Dec 19, 2023

View reviewed changes

drodzewicz requested review from kchelstowski and alannadolny December 20, 2023 14:42

jmiranda approved these changes Dec 21, 2023

View reviewed changes

drodzewicz added 3 commits December 22, 2023 17:19

OBPIH-5840 Create a Dashboard indicator for number of requests waitin…

a655cb8

…g for approval

OBPIH-5840 Fix cachablity of indicator when switching users

30ee7d0

OBPIH-5840 refactor requestPendingApproval method to use createCriter…

9f9c76f

…ia intead of pure SQL and build redirect URLs with createLink

drodzewicz force-pushed the OBPIH-5840-dashboard-indicator-for-requests-waiting-for-approval-approvals-lists-pages branch from 86eb0bc to 9f9c76f Compare December 22, 2023 15:22

drodzewicz changed the base branch from feature/upgrade-to-grails-3.3.10 to release/0.9.0-hotfix1 December 22, 2023 15:23

drodzewicz removed the warn: do not merge Marks a pull request that is not yet ready to be merged label Dec 22, 2023

awalkowiak approved these changes Dec 22, 2023

View reviewed changes

awalkowiak merged commit 09c8dce into release/0.9.0-hotfix1 Dec 22, 2023
1 check passed

awalkowiak deleted the OBPIH-5840-dashboard-indicator-for-requests-waiting-for-approval-approvals-lists-pages branch December 22, 2023 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5840 Create a Dashboard indicator for number of requests waiting for approval #4424

OBPIH-5840 Create a Dashboard indicator for number of requests waiting for approval #4424

drodzewicz commented Dec 18, 2023

drodzewicz commented Dec 18, 2023

alannadolny Dec 19, 2023

kchelstowski Dec 19, 2023

drodzewicz Dec 20, 2023

drodzewicz Dec 20, 2023

drodzewicz Dec 20, 2023

kchelstowski Dec 20, 2023

kchelstowski Dec 20, 2023

alannadolny Dec 19, 2023

kchelstowski Dec 19, 2023

drodzewicz Dec 20, 2023

kchelstowski Dec 19, 2023

kchelstowski Dec 19, 2023

OBPIH-5840 Create a Dashboard indicator for number of requests waiting for approval #4424

OBPIH-5840 Create a Dashboard indicator for number of requests waiting for approval #4424

Conversation

drodzewicz commented Dec 18, 2023

drodzewicz commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment