[ckan-dev] Organizations Merge

Tue Sep 18 14:46:24 UTC 2012

> 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 don't think it's ready to merge as it is as some things are quite broken
(e.g. groups appear on the organizations page), problems found so far
are in the pull request comments, and there aren't tests yet. But if we
make a plan and spend some dev time on it I think we could get it
merged.

I guess my main concerns would be:

- Lots of small problems that the three of us have found in code review,
  these could be fixed but needs some dev time

- If we're going to merge it then new tests for all this new code need
  to be written, and that's a fair chunk of dev time

- Not sure how happy I am with the way the existing tests have been
  changed to make them pass, e.g. some of the test users who were not
  sysadmins have been made sysadmins, some tests that previously
  required a 200 OK will not pass with either a 200 or a 403, etc.

- Perhaps we need to add more to the implementation section at the
  bottom of the spec, clarifying where we're intending to go with this,
  in terms of tests that need to be replaced or written, is the old auth
  stuff planned to be replaced, etc.

- Groups and organizations are different code paths at the logic,
  controllers and templates levels, but both share the same model, seems
  a bit messy to me, and there are things like a dataset has to have
  exactly one organization but what it actually has at the model level
  is a list of 0 or more groups, either implement them as one code path
  or as two

- There is a lot of code duplication. Removing groups and just going
  with organizations would fix this. If some instances really do want
  groups that are separate to and orthogonal to organizations, then they
  could fund this groups feature to be developed.

  If that comes up there will be the question of whether groups should
  be developed as a special-case of organizations sharing the same code
  base, or whether as a completely separate code path. I think it
  depends, in the current spec groups are maybe not different enough from
  organizations to justify their own code path, but the argument was
  that we might want the two features to evolve in different directions
  so didn't want to tie them to each other, as this would make it
  difficult to evolve the two features orthogonally and we'd end up with
  spaghetti code