EZP-30904: UDW: Sorting settings are ignored for starting location #209
Conversation
|
Sorry, not sure if failed checks have some relation to this PR |
| this.props.loadLocation( | ||
| { ...this.props.restInfo, locationId: this.props.startingLocationId }, | ||
| (response) => { | ||
| this.startingLocation = response.View.Result.searchHits.searchHit[0].value.Location; |
There was a problem hiding this comment.
I think this will fail if startingLocationId will be 1. In this case, there is no location.
There was a problem hiding this comment.
I checked changes locally and I have TypeError: Cannot read property 'value' of undefined for startingLocationId: 1; for others it works.
|
@dew326 @tischsoic can you please check it one more time? |
|
And the latest commit fixed the failed checks. |
| { ...this.props.restInfo, parentLocationId: this.props.startingLocationId }, | ||
| this.updateLocationsData | ||
| ); | ||
| this.handleRootLocations(); |
There was a problem hiding this comment.
I would name this handleStartingLocation()
| * @method loadRootLocations | ||
| * @memberof FinderComponent | ||
| */ | ||
| loadRootLocations() { |
| * @memberof FinderComponent | ||
| */ | ||
| loadRootLocations() { | ||
| const sortClauses = this.getLocationSortClauses(this.startingLocation); |
There was a problem hiding this comment.
| const sortClauses = this.getLocationSortClauses(this.startingLocation); | |
| const sortClauses = this.getLocationSortClauses(this.startingLocation); | |
| */ | ||
| handleRootLocations() { | ||
| // Starting location is already loaded or its default | ||
| if (this.startingLocation.id === this.props.startingLocationId) { |
There was a problem hiding this comment.
Please define a const with the root id, and check if this.props.startingLocationId === ROOT_LOCATION_ID
const ROOT_LOCATION_ID = 1
| handleRootLocations() { | ||
| // Starting location is already loaded or its default | ||
| if (this.startingLocation.id === this.props.startingLocationId) { | ||
| this.loadRootLocations(); |
There was a problem hiding this comment.
The this.setDefaultActiveLocations(); should be moved here as well.
| this.props.loadLocation( | ||
| {...this.props.restInfo, locationId: this.props.startingLocationId}, | ||
| (response) => { | ||
| if (response.View.Result.searchHits.searchHit[0].value.Location) { |
There was a problem hiding this comment.
| if (response.View.Result.searchHits.searchHit[0].value.Location) { | |
| if (response.View.Result.searchHits.searchHit.length) { |
| {...this.props.restInfo, locationId: this.props.startingLocationId}, | ||
| (response) => { | ||
| if (response.View.Result.searchHits.searchHit[0].value.Location) { | ||
| this.startingLocation = response.View.Result.searchHits.searchHit[0].value.Location; |
There was a problem hiding this comment.
Here we should not introduce a new variable but use the activeLocations state and add it to the state.
| @@ -186,10 +225,13 @@ export default class FinderComponent extends Component { | |||
| * @memberof FinderComponent | |||
| */ | |||
| onLoadMore(parentLocation) { | |||
| if (parentLocation === null) { | |||
There was a problem hiding this comment.
if you will add the location to the activeLocations changes in this method are not needed.
|
@dew326 1208669 implements all requested changes except getting rid of Could you please do the following changes (I hope for you it will take much less time then for me):
Alternatively, this PR could be merged, as it fixes the bug. And |
|
@SerheyDolgushev could you take a look and try it on your installation? It works for me but maybe I forgot some edge case. |
|
@dew326 just checked on my installation and looks fine |
|
Ok, so we will finish the review and start QA. |
|
@tischsoic could you merge it up? |
|
Merged up:
|
Please check https://jira.ez.no/browse/EZP-30904 for more details.