loans: date range search #783
Conversation
topless
commented
Mar 27, 2020
•
topless
commented
- filters loans_from_date and loans_to_date on loan start date
- datepickers for ui
- depends on react-searchkit release 0.18.0
- closes [Req] Statistics - Loans #168
|
Requires: inveniosoftware/react-searchkit#99 |
topless
force-pushed
the
168-date-filter
branch
5 times, most recently
from
73068cd to
7e4ca5d
Compare
topless
force-pushed
the
168-date-filter
branch
5 times, most recently
from
c51d111 to
f21b380
Compare
| @@ -30,8 +30,10 @@ beforeEach(() => { | |||
| }); | |||
|
|
|||
| const searchApi = new InvenioSearchApi({ | |||
| url: documentApi.searchBaseURL, | |||
| withCredentials: true, | |||
| axios: { | |||
There was a problem hiding this comment.
did it change because of axios or react-searchkit? (sorry if this was mentioned somewhere before)
|
|
||
| /** react-searchkit allows having the same filter multiple times. | ||
| * For the range dates filters we want each filter one time only so we have | ||
| * to remove any pre-existing filters with the same name |
There was a problem hiding this comment.
when does it happen that we have filters with the same name?
There was a problem hiding this comment.
Good question, I would re-phrase then given the confusion:
/** react-searchkit allows having the same filter multiple times with different values.
* For this range dates filters, we want each filter to appear only one time with one value
* (e.g. loan_start_date = `<date>`)
*
|
|
||
| /** react-searchkit allows having the same filter multiple times. | ||
| * For the range dates filters we want each filter one time only so we have | ||
| * to remove any pre-existing filters with the same name |
There was a problem hiding this comment.
Good question, I would re-phrase then given the confusion:
/** react-searchkit allows having the same filter multiple times with different values.
* For this range dates filters, we want each filter to appear only one time with one value
* (e.g. loan_start_date = `<date>`)
*
| let filters = newFilter; | ||
| // If value is empty we simply remove the filter otherwise if we have | ||
| // value we remove the filter and add the new one. | ||
| if (!_isEmpty(value)) filters = [[name, ''], newFilter]; |
There was a problem hiding this comment.
To remove the previously selected filter, shouldn't you pass the already selected value first?
E.g. currently I have loan_start_date:2020-01-01 if I select the date loan_start_date:2019-01-01, shouldn't we pass:
[['loan_start_date', '2020-01-01'], ['loan_start_date', '2019-01-01']]
There was a problem hiding this comment.
That was indeed my initial understanding, but in the searchkit the comparison we do not check for the value, we only check if it matches the starting part. So if there is any value we pass ['loan_start_date', ''] to clear it.
There was a problem hiding this comment.
Unfortunately this is not the case, see tests that you wrote too. This is related to this bug: it happens that since the string is empty, it clears out all filters selected for this filter. If we fix that bug, we might break this code.
Please change it to something like (pseudo-code):
onDateChange = newSelectedValue => {
const resetPreviousValue = getFromRSKCurrentQueryState()
const newFilters = [resetPreviousValue];
if (!isEmpty(newSelectedValue)) {
newFilters.push(newSelectedValue);
}
return this.props.updateQueryState({ filters: newFilters });
};
|
topless
force-pushed
the
168-date-filter
branch
3 times, most recently
from
3a89081 to
032c41b
Compare
| */ | ||
| onDateChange = newFilter => { | ||
| const [name, value] = newFilter; | ||
| if (_isEmpty(value)) |
There was a problem hiding this comment.
I don't understand this if. If empty, it means that before might had a value, so you should call updateQueryState with the previous value.
If it had no value even before, then just return, nothing to do.
I would simply remove this if. WDYT?
| ); | ||
|
|
||
| let filters = [newFilter]; | ||
| if (!_isEmpty(oldFilters)) filters.unshift(_flatten(oldFilters)); |
There was a problem hiding this comment.
I am not sure I understand all this manipulation of values and different cases.
Here my reasoning:
case 1:
- current: []
- newFilter: ['loan_start_date', '2020-01-01']
- call
this.props.updateQueryState({ filters: [ ['loan_start_date', '2020-01-01'] ] });
case 2:
- current: ['loan_start_date', '2020-01-01']
- newFilter: ['loan_start_date', '2021-02-02']
- call
this.props.updateQueryState({ filters: [ ['loan_start_date', '2020-01-01'], ['loan_start_date', '2021-02-02'] ] });
In any case, we always pass the previous value to remove it, and eventually if a new one is set, add it.
onDateChange = newFilter => {
const [name, value] = newFilter;
const newFilters = this.props.currentQueryState.filters
// if there is a new date, add it to the current filters so that the previous date is removed and the new one is added.
// if the date is cleared, then just leave the previous date in the filters so that it will be remove
const hasSelectedNewDate = !_isEmtpy(value);
if (hasSelectedNewDate) {
newFilters.push(newFilter)
}
return this.props.updateQueryState({ filters: newFilters });
}
There was a problem hiding this comment.
@ntarocco I see what you mean and I am sorry for the confusion it was very messy I cleaned it up according to your suggestions, thanks!
- filters loans_from_date and loans_to_date on loan start date - datepickers for ui - depends on react-searchkit release 0.18.0 - closes inveniosoftware#168
- converted filters to post_filters - config expects an axios property - tests
