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

Subscriber adapters not supported in clients listing #1418

Merged
merged 4 commits into from
Aug 9, 2019

Conversation

xispa
Copy link
Member

@xispa xispa commented Aug 8, 2019

Description of the issue/feature this PR addresses

The custom ClientFolderView was overriding before_render and folder_item functions, but without calling their respective functions from the base class. Thus, subscriber adapters where
not considered in clients listing.

Current behavior before PR

Client listing cannot be extended or its behavior changed by means of subscriber adapters.

Desired behavior after PR is merged

Client listing can be extended or its behavior changed by means of subscriber adapters.

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

xispa added 2 commits August 9, 2019 00:23
The custom ClientFolderView is overriding before_render and
folder_item functions, but without calling their respective
functions from the base class. Thus, subscriber adapters where
not considered.
@@ -132,6 +132,9 @@ def __init__(self, context, request):
def before_render(self):
"""Before template render hook
"""
# Call `before_render` from the base class
BikaListingView.before_render(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind to use super here instead of calling the named base class directly?
E.g. like here: https://github.com/senaite/senaite.core/blob/master/bika/lims/browser/worksheet/views/folder.py#L205

# Call the folderitem method from the base class
item = BikaListingView.folderitem(self, obj, item, index)
if not item:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also use the super call here instead of calling the named base class, e.g.:
https://github.com/senaite/senaite.core/blob/master/bika/lims/browser/worksheet/views/analyses_transposed.py#L70

Btw, when can this happen that the item is None?
The thing with None values here is that the listing count would be wrong and I'm not sure if this is handled gracefully in senaite.core.listing, because it expects there dicts as folderitems

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, when can this happen that the item is None?

Indeed, it should not happen. And if it somehow happens, better to handle that as bug than obfuscating the anomaly here. So I agree with you

folderitem function is called by the folder_items function from
the base class, that is already in charge of calling the function
folderitem from subscriber adapters
@xispa
Copy link
Member Author

xispa commented Aug 9, 2019

@ramonski just noted that there was only the need to call the function before_render from the base class, cause folderitem function is called by folder_items function from the base class, which in turn, it calls the folderitem function of matching subscriber adapters already.

@ramonski ramonski merged commit 130c452 into master Aug 9, 2019
@ramonski ramonski deleted the client-folder-adapters branch August 9, 2019 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants