See http://drupal.org/node/1669924#comment-6191058 for some background.

I reckon we need:

- a new form for attaching an existing entity
- data sets define whether they allow this (as some, eg p2, don't)
- data sets define whether they allow creating a new attached entity (as that doesn't always make sense)

CommentFileSizeAuthor
#6 dataUIbuild.patch45.89 KBrlmumford
#2 1669982.party_.data-set-actions.patch20.61 KBjoachim

Comments

joachim’s picture

joachim commented
Title: move distinction between creating and selecting a user up to party core » add a data set actions system

So I scratched my head over this this morning, and thought that adding two actions to each dataset, and form callbacks for them, would result in a ton of new form functions, and page callbacks that have to find the right form builder, and then we'd have to provide some default form builders too for good DX and the function names would multiply like crazy. Especially as where we have two actions, we may want more.

So here's the idea in a nutshell:

Data set info defines actions that you can perform on their entities. Here's some examples:

- profile2 can:
-- 'add' a new entity
- user can:
-- 'attach' an existing user
-- possibly 'add' a new user too -- TODO!

When defining actions, data set info must also give the controller for this. This is a pretty simple class that wraps up the form handlers and a little bit more. Party code provides two default controllers which data sets can use:

- PartyDefaultDataSetUIAdd: add a new attached entity
- PartyDefaultDataSetUIAttach: attach an existing entity

Getting these actions in the UI is done by:

- party_menu_local_tasks_alter() looks at all of a data set's defined actions
- party_attached_entity_action_form() is a general dispatch point page callback which gets the right controller, and gets the form from it.

So that's the new architecture.

Here's the actual situation on the ground, patch imminent:

- PartyDefaultDataSetUIAttach is not implemented and only a stub class, because so far the only thing we know how to attach is users. Enter PartyUserDataSetUIAttach.
- PartyDefaultDataSetUIAdd is not implemented and only a stub class, because this is currently handled by party_attached_entity_edit_form() and I feel such refactoring belongs to a follow-up because this code change is complex enough already. That means that data sets declare the 'add' action in order to have the local action appear, but the controller they give for it is irrelevant ;)

Here's the planned follow-ups, in rough order:

- make a data sets actions define access. If you don't declare the 'add' action, then party_access() should reflect that this action is not allowed.
- make local action links for data set actions reflect cardinality: #1669758: add entity link in party_menu_local_tasks_alter() doesn't consider singleton/cardinality
- move 'detach' into a data set action
- add 'delete' as a data set action
- refactor 'edit' as a data set action

joachim’s picture

joachim commented
Status: Active » Needs review
StatusFileSize
new 1669982.party_.data-set-actions.patch20.61 KB

Here's the patch.

Though with Rob out of range in the US, I'm going to commit this, as it's a big step in fixing user integration.

I'll merge my branch in so we have the individual steps.

joachim’s picture

joachim commented
Status: Needs review » Fixed

Issue #1669982 by joachim: Add a system for data sets to define their own actions.

Clear caches LOTS -- this adds new classes which the registry needs to pick up.

joachim’s picture

joachim commented

Follow-ups:

#1673604: make a data set's actions define access to matching $op in party_access()
#1669758: add entity link in party_menu_local_tasks_alter() doesn't consider singleton/cardinality
#1673606: move 'detach' into a data set action
#1673608: create 'delete' as a data set action
#1673610: refactor attached entity edit as a data set action

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Title: add a data set actions system » Data Set Actions System
Status: Closed (fixed) » Active
StatusFileSize
new dataUIbuild.patch45.89 KB

Thought it was best to build on this here, rather than create a whole new issue.

I've been working on the data set actions stuff for a couple of weeks now, and the process of re-factoring I've gone through has reduced the size of the party code significantly, made the forms stuff much more robust, and given sub-modules such as party_user and party_simplenews much more flexibility.

The patch is a bit of a monster one and includes a number of the follow up issues in it. Here is a list of what's in this patch.

In this patch I have:

  • moved detach into a data set action
  • moved add into a data set action
  • moved edit into a data set action
  • moved view into a data set action (!)
  • extended the PartyDefaultDataSetController to make actions easy to get to
  • refactored alot of the forms stuff around this, most notably:
    • Removed the 'form callback' key from data set info
    • Replaced 'party_attached_entity_edit_form', 'party_attached_entity_remove_form' and others with party_data_set_action_form.
    • Rewrote party_data_set_attach_form to accept different actions for different data sets.
    • Rewrote the content type plugins for viewing and editing data sets.
    • Implemented hook forms. 'party_data_set_action_form__{data_set_name}__{action}' is now the form_id even though this still calls party_data_set_action_form and either can be used in hook_form_form_id_alter.
    • Removed the old attached entity form callbacks
  • Updated data set action paths to effectively all be the same, see party_data_set_action_page and party_data_set_action_page_title for more info

Things still to do/ Follow up:

  • Consolidate stuff in party_menu so action links are generation automatically based on what actions are available for what data set.
  • Give actions control over access: Add a hook_permission method so that then can define permission and a actionAccess method for them to grant deny or say nothing about access
  • Provide an all purpose 'data_set_action' content_type plugin for panels and deprecate the edit form and view ones.
  • (Maybe) Provide a way for the Data Action classes to provide a UI for either the WHOLE data set or be limited to one $delta at a time. e.g. The remove action only makes sense when dealing with one attached entity at a time whereas the edit action can provide one form for many attached entities. User may want to do something special and simplenews definitely wants to do something different. I'm thinking of a behaviour similar to how you can set 'FIELD_BEHAVIOUR_CUSTOM' on field widgets.
  • Remove $party and $data_set as parameters from all of the methods in side an action controller. These are now passed in to the constructor, so having them there is somewhat redundant. This would make the DX alot nicer.
  • Update party_user module to work with latest changes.

Some examples of how this works

To embed a data set action form programmatically, all you need to do is:

$data_set_controller = party_get_data_set($party, 'party_main_profile');
$data_set_controller->getAction('edit')->build($party, $data_set_controller);

To get the title of a particular action, all you need to do is:

$data_set_controller = party_get_data_set($party, 'party_main_profile');
$data_set_controller->getAction('edit')->actionTitle($party, $data_set_controller);
rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Assigned: Unassigned » joachim
Status: Active » Needs review
joachim’s picture

joachim commented
Status: Needs review » Needs work

> $data_set_controller->getAction('edit')->build($party, $data_set_controller);

Er... you're calling on the object, and then PASSING the object?
Sorry, but that looks really weird.

> The patch is a bit of a monster one and includes a number of the follow up issues in it. Here is a list of what's in this patch.

Could this be broken up into smaller issues please?

joachim’s picture

joachim commented
Status: Needs work » Fixed

A few things that stick out at the start of the patch, but as you say, it's a monster.

But as I say -- multiple issues for all this please. Start with building the framework you need, and then converting the actions.

+++ b/includes/party.data.inc
@@ -282,6 +288,28 @@ class PartyDefaultDataSet {
+    $info = party_get_data_set_info($this->data_set);

There should be a helper for this. Or rather, doesn't the data set class already hold its own info?

+++ b/includes/party.data.inc
@@ -282,6 +288,28 @@ class PartyDefaultDataSet {
+    $this->actions[$action] = new $info['actions'][$action]['controller']($this, $action);

You're giving the data set controller to the action. So it never needs it again -- see my previous comment.

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented

But as I say -- multiple issues for all this please.

Yeah, that would be preferable. Hard to find blocks of it that work without the whole. But will endeavour to do so.

> $data_set_controller->getAction('edit')->build($party, $data_set_controller);

Er... you're calling on the object, and then PASSING the object?
Sorry, but that looks really weird.

I totally agree, its in my list of things to do in comment 6 =)

joachim’s picture

joachim commented

> I totally agree, its in my list of things to do in comment 6 =)

Right, but let's please not commit code that's in that shape!

If your new actions depend on the refactoring, I suggest making patches for the refactoring first.

> . Hard to find blocks of it that work without the whole

I trust you've been working on a local git branch for this? ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.