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

Merge of extensible_properties branch #11

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Gagaro
Copy link
Member

@Gagaro Gagaro commented Nov 19, 2013

The idea behind this work was to be able to fetch any attribute using member properties. This branch is in a project in development for 4 months now, we didn't encounter any issue.

It also includes french translation and a few fixes.

The only thing which could be an issue is the deletion of "description='bio'" from the properties_map. As stated in the file, it can be RichText and thus break the module.

So I think we should talk about it before merging this pull request. I think a solution would be to rename the bio attribute to description and add description as a default element of the whitelist. It is not the best solution as it would break implementation directly using the bio attribute of the member content though.

title=u'Local Roles',
description=u'The list of additional local roles members will be granted in the context of their own profile objects',
title=_(u'Local Roles'),
description=_(local_roles_desc),
Copy link
Member

Choose a reason for hiding this comment

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

These are not picked up by i18ndude. If I run the rebuild_i18n.sh in the dexterity/membrane directory, two message ids are removed from the .po files. I would say: just use a label and a default and use the least indentation possible. Always a bit messy with such long messages, I agree. :-/

@mauritsvanrees
Copy link
Member

Sorry for waiting so long before having a look at this.

I like this, it could be useful.

About the bio: I don't see why richtext would pose a problem. If I undo that part of the changes on this pull request in behavior/membraneuser.py and tests/test_member.py, the tests still pass. If you have seen problems with it, can you tell me how to reproduce those? A test would be good, if possible.

Is there any way that I can see within a Plone Site that it actually works? I am trying to add a template or browser view where I can see member properties. I have a pdb in getPropertiesForUser and it never gets called. It should be easy, but it has been a while since I have written code like that. :-)

Can you add a test that actually uses a new property?

@Gagaro
Copy link
Member Author

Gagaro commented Jan 15, 2014

Our module use RichText for the bio attribute: https://github.com/collective/collective.rcse/blob/master/collective/rcse/content/member.py#L113.

The error came from https://github.com/do3cc/Products.PluggableAuthService/blob/master/Products/PluggableAuthService/UserPropertySheet.py#L36.

I'll try to add a test and fix the i18n when I'll have some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants