Skip to Content.
Sympa Menu

comanage-dev - Re: [comanage-dev] Re: (CO-89) Add sponsor to data model

Subject: COmanage Developers List

List archive

Re: [comanage-dev] Re: (CO-89) Add sponsor to data model


Chronological Thread 
  • From: Benn Oshrin <>
  • To: comanage-dev <>
  • Subject: Re: [comanage-dev] Re: (CO-89) Add sponsor to data model
  • Date: Thu, 24 Nov 2011 23:12:39 -0500

On 11/22/11 7:58 PM, Marie Huynh wrote:

In sponsorList(), why do you need to explicitly join to get Names? Doesn't
Cake automagically pull them as associated data?

No, it doesn't pull all of them in, just the associated data for the
current one being edited.

If you do a find('all'), cake should return the same amount of associated data for all returned items, so either names for all records or names for no records. Use of containable behavior (or less desirably recursive=2) should retrieve names if they're not being retrieved by default.

You shouldn't have to use a join here.

So if sponsor applies to a role, why is it displayed in co_people?

You added a chart of Role Attributes to the co_people edit page.
https://skitch.com/mariehuynh/gk2gm/comanage-registry-edit-co-person

I'm adding a column to that chart.

OIC... that "Role Attributes" list is supposed to be a summary table of attributes. Clicking on view/edit takes you to the detailed view. I don't think we need sponsor in the summary table.

+++ trunk/app/controllers/co_person_roles_controller.php

Why do you need to loadModel('CoPerson')? Shouldn't it already be available
to CoPersonRoles due to the association?

No, it didn't work until I loaded that model.

CoPerson hasMany CoPersonRole, so the model should be accessible without additional work.

Instead of

$this->loadModel('CoPerson');
$this->set('sponsors',$this->CoPerson->sponsorList());

You should just be able to do

$this->set('sponsors',$this->CoPersonRole->CoPerson->sponsorList());

You should do a print_r($this) (in any controller, not necessarily right here) at some point and take a read through what Cake makes available by default.

Also, it looks like you copy and pasted isAuthorized from a controller with
more logic than Demographics needs. You should review what's going on there.

Similarly, editself().

I was still working on the demographics files when I did the diff for
co89. Sorry if that wasn't clear. I think demographics does need
editself so that it will redirect people to their own edit page so
they can can enter their own information. Is there a better way of
doing that?

Well editself is just a means to redirect off the main menu page to the correct org_identity (which, incidentally, I need to fix for CO-199). One question is how we want people to access their demographic data. It could hang off the main menu, or it could be accessed via their CO Person data, at which point we'd know what $co_demographics_id is theirs and could just embed it in the link.

Ultimately this all gets redone for the Fancy New UI, so I'd be inclined to skip the editself redirect and just hang it off view/edit CO Person for now.

+++ trunk/app/libs/lang.php

In the en.foo translations, key them on the Enum values. eg

'en.gender' => array(GenderEnum::Female => 'Female'

Is there a reason I shouldn't do this with en.affil while I'm working
on CO 87(Affiliation model cleanup)? I see it used in these files:

libs/lang.php: 'en.affil' => array('faculty' => 'Faculty',
models/cmp_enrollment_configuration.php: 'select' =>
array('options' => $cm_texts[ $cm_lang ]['en.affil'],
views/co_person_roles/fields.inc:
$cm_texts[ $cm_lang ]['en.affil'],

Do you have any plans for it I should know about before changing it?

CO-170 covers this.

The potential gotcha here is that new style Enums are all short letters (eg: M/F) as we discussed on the call, whereas old style affils are words (eg: faculty/staff). We'd have to make sure we understood what existing code was doing before we changed that.

I think I originally intended from_api/to_api to address this somehow, but that's definitely half-baked (and therefore commented out).

Why do you define fd.de.persid? (See related comment below.)

It's displayed as a header for the chart of all demographic data in
place of the name so it can remain confidential. The column and this
could be removed altogether, but it makes for a convenient link while
testing.

As a general rule, we don't display row IDs. As an alternative, see how the roles are rendered on the co_person view (discussed above) using 1,2,3,....

I suspect the index view for co_demographics is as useful as the index view for telephonenumbers is... ie: not. We could contemplate tossing these as part of the Fancy New UI.

For fd.sponsor.desc, the convention is not to use parens, but to render in
<font class="desc">. See view/cmp_enrollment_configurations/fields.inc as an
example. (Applies to views/co_person_roles/fields.inc and
views/co_people/fields.inc.)

I was trying to make it match the other fields already on the page.
Should I change those to match the new convention too? See:

https://skitch.com/mariehuynh/gk3xf/comanage-registry-edit-co-person

As discussed above, on co_people that column should go away.

But yeah, those older examples should probably be fixed (as a separate commit).

-Benn-



Archive powered by MHonArc 2.6.16.

Top of Page