fix error with collectionfilter=1 not being included in batch links #65
Conversation
| @@ -9,6 +9,6 @@ def set_content_filter(context, event): | |||
| req = event.request | |||
| if 'collectionfilter' not in req.form: | |||
| return | |||
| del req.form['collectionfilter'] | |||
| # We leave collectionfilter=1 in req.form so that it gets put into batch links | |||
There was a problem hiding this comment.
The reason for removing the collectionfilter url parameter was to exit early and not have to make_query for each traversal step - which can be a lot.
Maybe we should instead check for the existance of event.request['contentFilter'] - if possible?
|
Ok but I wasn't sure there was multiple traversals?
…On Sat, 6 Jul 2019, 02:07 Johannes Raggam, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/collective/collectionfilter/contentfilter.py
<#65 (comment)>
:
> @@ -9,6 +9,6 @@ def set_content_filter(context, event):
req = event.request
if 'collectionfilter' not in req.form:
return
- del req.form['collectionfilter']
+ # We leave collectionfilter=1 in req.form so that it gets put into batch links
The reason for removing the collectionfilter url parameter was to exit
early and not have to make_query for each traversal step - which can be a
lot.
Maybe we should instead check for the existance of
event.request['contentFilter'] - if possible?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#65?email_source=notifications&email_token=AAAKFZHZ6MPLL5EOY4BUPITP56LYPA5CNFSM4H6J4F6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5UMBII#pullrequestreview-258523297>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAKFZFTWOVPC5HZMTCN253P56LYPANCNFSM4H6J4F6A>
.
|
|
@djay I'd have to step in myself and see how often this is typically called. There is a chance that this is only a micro optimization but this code is called for all path-parts (IIRC). |
|
@thet I added the check but not sure what you all path parts you mean. its still skipping if collectionfilter=1 isn't in there so it would only other parts of collectionfilter traversals that would call make_query. Also, any idea why 5.2 is failing? I don't think it's something I did |
|
@thet Given this is a major bug and breaks batching can we get this merged and then worry about optimisations later? |
This reverts commit c21400e.
|
@djay can u resolve the conflicts? I'll merge this one if tests are green. |
|
@petschki failing due to merging my reversion that means the rebase also removes the code :( |
|
@djay I've fixed another missing code snippet due to your rebase. Local tests are now green. I wait for travis and merge later. |
No description provided.