[ckan-dev] Organizations Merge

Tue Sep 18 14:38:16 UTC 2012

On 18 September 2012 15:03, David Raznick <david.raznick at okfn.org> wrote:

> Hello All
>
> As my internet has been patchy, I also did a code review of the
> organizations branch.  My take on the important to look at are.
>
> *solr schema*
>
> schema 1.4 should not be changed
> I think we should we keep groups in the new schema.
> If we are doing backwards compatible changes should we call
> "organizations" "organization".
>
>
> *package controller*
>
> Auto make of packages for orgs not working due to comment. line 477
> FIXME.  Looks like the error could be as simple as changing
> organization__0__name to organizations__0__name but this needs sorting
> out.
>
> Dodgy ignore_auth calls seem like a hack to make test work. (this
> looks like the main blocker)
>
> *dictization*
>
> I think we should remove packages associated with organizations when
> doing orgainzation_dictize and organization_save.
> We are using IGroup controller still for orgs.
> Helpers facet_titles deprecated but seen to have been brought back to life.
>
> *validation schema*
>
> remove packages completely from organization schema
>
> *users form*
>
> indentation incorrect users_form.html
>
> *auth functions*
>
> Would be good if this was thoroughly looked through to make sure we
> are not doing anything very wrong.  Tests would not go a miss here.
>
>
> All in all I think we should try and merge it, even though it makes
> more of a mess of the code base.  I think that we can clean this up
> possibly by getting rid of groups entirely.  It may have been a
> mistake to make a new entity and just kept with groups and renamed
> them organizations.  I do not think it was foreseen how much
> duplication there would be.
> I also think we have to live with the UI tests breaking the legacy_ui
> tests even though it is a shame that Toby spent a lot of time making
> sure they worked before the demo merge.
>
>
I have to say that I remain unhappy with this whole patch set.

To me organisations and groups are pretty much the same the only
differences being that datasets are owned by a single organisation which
affects the authorisation of users accessing/changing that dataset

maybe that is an oversimplification but I cannot see any other real
difference (maybe the dataset view gets altered due to the org it is owned
by)

as we already have different group types then why not just use this
functionality?

This feels like quite a simple change whereas the branch seems to be just
lots of code duplication for no gain

I do not understand why any of the tests would be broken by this

Also it is my understanding that the auth changes are not going to be
implemented (not sure I understand the logic behind this decision as it was
the driving force for this work anyway)

So my questions would be why are we not just making a couple of changes to
package to allow org ownership and just creating an extra group type (orgs)?

Toby

> Thanks
>
> David
>
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20120918/58055fdd/attachment-0001.html>