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

OBPIH-5814 Add NONE option to approvers filter #4323

Merged
merged 3 commits into from
Oct 11, 2023
Merged

Conversation

drodzewicz
Copy link
Collaborator

@drodzewicz drodzewicz commented Oct 10, 2023

Unfortunately, the approach with the subqueries that we discussed on the tech-huddle wasn't ideal, the issue that came up was with filtering by requests with empty associations, so I decided to go with the simplest solution of querying the request ids and then querying the requests by the list of the these ids.

the approach with subqueries looks something like

  if (params.list("approver")) {
      or {
          createAlias("requisition", "requisition")
          if (params.list("approver").contains("null")) {
              isEmpty("requisition.approvers")
          }
          if (stockMovement.approvers) {
              createAlias("requisition.approvers", "approver")
              add Subqueries.exists(
                      DetachedCriteria.forClass(Person.class)
                              .add(Restrictions.in("approver.id", stockMovement.approvers.id))
                              .setProjection(Projections.property("id"))
              )
          }
      }
  }

The issue I mentioned came up when trying to filter by None and some other users. It did not return any requests with empty approvers because createAlias("requisition.approvers", "approver") by default sets the association to INNER_JOIN.
When changing the jointype on Approvers to LEFT_JOIN, we face the same problem as in the beginning, of approver column missing the rest of the data (other than specified in filter approvers)

This issue started quite the discussion of the functionality/logic and UX of filtering by empty fields values, so I think we will have to come back to this discussing and come up with a final verdict.

For now I think we should follow the approach of how it is done on Purchase orders with paymentTerms filter.


def stockMovementsIds = OutboundStockMovementListItem.createCriteria().list() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest adding a comment above with the name of the ticket:
"OBPIH-5814: Because stock movement approvers could be filtered out from the requisition by given approvers in params, here we only return ids of stock movements and below we had to make another query to get stock movements with given id's, so that second query doesn't anymore cut any approvers"

or something similar.
I won't lie that even though I knew what you've done here, I had to think a bit when looking at the code of what is going on and how it is working.

@@ -148,6 +152,11 @@ class OutboundStockMovementService {
'in'("shipmentType", shipmentTypes)
}
}
}

// Get result count
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Get result count" is not accurate I'd say, because we also return stock movements from there, not only "total count" (which I think you meant here).

Here I'd add a short note: "This query is about to return data with filters applied from the query above, but this will not filter out any approvers from the result. See the comment above".

@kchelstowski
Copy link
Collaborator

besides adding a comment/comments, lgtm.
I wish I had more time to help, maybe it'd be possible to figure it for all cases mentioned by you, using Restrictions/Subqueries approach.

@awalkowiak awalkowiak merged commit fcc5238 into develop Oct 11, 2023
3 checks passed
@awalkowiak awalkowiak deleted the OBPIH-5814 branch October 11, 2023 08:48
awalkowiak pushed a commit that referenced this pull request Oct 26, 2023
* OBPIH-5814 Add ability to filter by none approvers

* OBPIH-5814 Include all approvers when filtering requests by approver

* OBPIH-5814 Add comments explaining the solution
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.

None yet

3 participants