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

Issue #92 Display comittee member information as per requirement #103

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

Conversation

devilankur18
Copy link
Collaborator

No description provided.

@devilankur18
Copy link
Collaborator Author

@anandology please review, I may have interpreted the ticket wrongly.

@devilankur18 devilankur18 changed the title Fixed #92 Display comittee member information as per requirement Issue #92 Display comittee member information as per requirement Sep 3, 2015
incharges = filter(lambda m: m.role.role == u'Incharges', c.committee_members)
return incharges


Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only troubling part.

The committee slug will be different for different levels. The Role for Incharge could be different as well.

I think it is better to take it from a config setting or the first available committee. Lets have a function to get the committee name and role. That'll return first available committee and first role from that, unless a config setting is specified. Lets use WARD_MASTER_COMMITTEE and WARD_MASTER_COMMITTEE_ROLE with WARD substituted with appropriate level name.

Also, In python it is idiomatic to use list comprehensions instead of map/filter. The usual practice is:

incharges = [m for m in c.committee_members if m.role.role == 'Incharges']

- Showing incharges for every children place, right away in the list.
- All the comittees and their memebers with contact info shown right away in comittees page.
com = self.get_primary_committee()

if com:
committee_role_slug = app.config.get("%s_MASTER_COMMITTEE_ROLE" % self.type.short_name.upper(), com.type.roles.pop().role)
Copy link
Contributor

Choose a reason for hiding this comment

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

com.types.roles.pop() looks a bit scary. com.types.roles[0] is safer I think.

Don't worry, I'll take care of that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@anandology I think I tried that, but its was throwing some errors, as it was not an array some custom form for list.

@anandology
Copy link
Contributor

@devilankur18 I've added link a mockup in #92. Please have a look at it and let me know your comments there.

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.

2 participants