OBPIH-6991 Add multi-select product filter to consumption report#5560
OBPIH-6991 Add multi-select product filter to consumption report#5560alannadolny merged 5 commits intodevelopfrom
Conversation
alannadolny
requested review from
SebastianLib,
awalkowiak,
ewaterman and
kchelstowski
alannadolny
added
the
warn: do not merge
github-actions
bot
added
type: feature
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5560 +/- ##
============================================
- Coverage 9.12% 8.53% -0.59%
+ Complexity 1170 1112 -58
============================================
Files 701 702 +1
Lines 45281 45335 +54
Branches 10851 10867 +16
============================================
- Hits 4131 3869 -262
- Misses 40497 40887 +390
+ Partials 653 579 -74 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (command.selectedProducts) { | ||
| List<String> selectedIds = command.selectedProducts*.id | ||
| command.rows.keySet().removeAll { row -> !(row.id in selectedIds) } | ||
| } |
There was a problem hiding this comment.
if you're just using the ids, not the whole product itself, why not have the command take a List<String> selectedProductIds so that you don't need to bind them to the real objects?
EDIT: on second thought, I think the way you have it is fine. I doubt it's a huge performance difference to look up a handful of domains by ids.
| </div> | ||
| <div class="filter-list-item"> | ||
| <label> | ||
| <warehouse:message code="consumption.products.label" default="Products"/> |
| data-testid="product-select" | ||
| multiple="true" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
heads up that I modified this slightly in my PR. You should only need to do:
<g:selectProductAjax
id="..."
value="${command?.selectedProducts}"
multiple="true"
/>
| String text = val.name ?: val.toString() | ||
| "<option value='${id}' selected='true'>${text.encodeAsHTML()}</option>" | ||
| }.join("\n") | ||
| } |
There was a problem hiding this comment.
why do you need to build the options yourself? For me it auto-generates them:
There was a problem hiding this comment.
Those options that I added are used for pushing params to the URL:
it looks like those options generated by jquery are not appended to the URL (?)
There was a problem hiding this comment.
I wonder if instead of adding new elements, you could modify the existing options. From my screenshot, it already has the options elements with a value attribute correctly set to the id. It looks like the only difference in your code is that you add a selected='true' attribute. Is that what's required for you to get it in the url?
alannadolny
force-pushed
the
ft/OBPIH-6991
branch
from
14402c3 to
a392f02
Compare
| Boolean isCsvReport = params.format == "text/csv" | ||
| List<Object> data = reportService.getTransactionReport(location, categories, tagList, catalogList, startDate, endDate, isCsvReport) | ||
| List<Object> data = reportService.getTransactionReport( | ||
| location, |
There was a problem hiding this comment.
Could we move the location and categories fallback to the service (or data binding in command) and just pass the command instead location, categories, producs, start, and end date? Additionally are we able to move params into the command too?
alannadolny
force-pushed
the
ft/OBPIH-6991
branch
from
a392f02 to
b8e20e9
Compare
3 participants
This PR is created based on OBPIH-6933, so I set it as a base branch for easier review.