Posted by thekevinday on September 20, 2011 at 2:26pm
18 followers
| Project: | Workbench Access |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
This adds support for:
- Drupal Core Actions
- Rules Module
- Workbench Moderation State Triggers
While adding rules and actions integration to workbench_module, I realized that I needed the same things for workbench_access.
I converted my work from the following threads:
- https://drupal.org/node/1226688
- http://drupal.org/node/1079134 (the actions code that I added)
I added support for triggers based on (and using some of the code from):
- http://drupal.org/node/1079134
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| workbench_access-rules_actions_triggers_integration-1.patch | 11.73 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 62 pass(es). | View details | Re-test |
Comments
#1
Forgot to set it to "needs review".
#2
This update does the following:
- removes a drupal_set_message() debug statement I forgot to remove.
- defines the provided variable 'workbench_access_states' as an array of taxonomy terms instead of the type called 'unknown'.
#3
* Changes the moderation state for a given node.Huh?
Looks like some of the code comments were copied from another patch.
This is pretty awesome. Does it support VBO as well?
#4
Re-rolled patch with the "git diff" command – it can now be applied using "git apply". (All cred to thekevinday.)
#5
I never tried with VBO (view bulk operations, right?).
The patches I made are with git diff, but I am using the 7.x-1.x branch and not master.
Should I be rolling the patches against master instead of 7.x-1.x?
I fixed the comments to be for workbench_access and not workbench_moderation and also added more description about what the functions do.
I noticed that I was using 'state' and 'states' everywhere. This is because workbench_moderation has 'states'.
I changed the code to use 'section' and 'sections' instead of 'state' and 'states'. This should make it more consistent. The downside is it will mean any existing rules from the previous patch might need to be re-created. A cache-clear may be necessary as well.
There is another area I forgot to address. Workbench access supports multiple kinds of access, namely taxonomy based AND menu based. What I wrote is for taxonomy based. Do I need to do anything differently for menu based and if so, what?
Edit: I noticed a little late that I was searching for 'state' with case-sensitive on.
I found one-more area to cleanup:
line #104 of workbench_access.rules.inc,
'label' => t("Workbench Access States"),should become'label' => t("Workbench Access Sections"),I will add the change to the next patch as I am sure there will be things to fix or add once this current patch gets reviewed..
#6
7.x-1.x is the proper branch.
There should be abstraction functions that handle the different access schemes for you. I'll have to look at how Rules actually works before giving you any guidance.
#7
Thanks for the updated patch. Switching to 7.x-1.x instead of master.
(@agentrickard: Realizing I've recorded screencasts with an old version – and with old knowledge – I will re-record the one about WB Access today. It will be published tomorrow.)
#8
The patch isn't applying anymore against the latest dev version. It is working with the version from 2011/09/19 the commit before the patch was posted.
http://drupalcode.org/project/workbench_access.git/commit/778894951b3541...
Checking patch workbench_access.module...
Hunk #1 succeeded at 1280 (offset 95 lines).
error: while searching for:
$info = module_invoke_all('workbench_access_info');
return isset($info[$scheme]) ? $info[$scheme] : FALSE;
}
error: patch failed: workbench_access.module:1729
Checking patch workbench_access.rules.inc...
Maybe @thekevinday could repost a new version.
#9
Here is a new version of #5 which should work with the latest dev. I just copied over the changes from #5 by hand to the latest dev and recreated the patch. So there is no gurantee that the code is still working, I will test the next days.
#10
The last submitted patch, 1285110-9-rules_actions_triggers_for_workbench_access.patch, failed testing.
#11
That test fail is not actually related to this patch, and is a problem with grabbing a random user from an array.
#12
#9: 1285110-9-rules_actions_triggers_for_workbench_access.patch queued for re-testing.
#13
This patch is not working for me. edit: (patch from #9 btw)
Rule I created:
Event: Before Saving Content
Condition: Contents current access control section
Actions: 1. Assign access control section (presave)
2. Unpublish content
Neither of the 2 actions are completed when I edit/save the node. Also I'm getting 2 notices when I save the node:
Notice: Undefined property: stdClass::$workbench_access_id in workbench_access_rules_condition_contents_current_access_section() (line 148 of /www/sites/all/modules/workbench_access/workbench_access.rules.inc).
Notice: Undefined property: stdClass::$workbench_access_id in workbench_access_rules_condition_contents_current_access_section() (line 152 of /www/sites/all/modules/workbench_access/workbench_access.rules.inc).
#14
The code in question crashes because the property workbench_access_id does not exist in the node object.
First, it may be a good idea to ensure that the said property exists before acting on it.
That is easy to do with a new patch.
Second, the real issue is that the said property does not exist.
It should exist, in fact, because it does not exist we have the following to scenarios that I can think up at the moment:
1) The $node variable that is passed to the function is not a valid node object.
2) The hook_node_load() for workbench_access is not being called and thereby is not making the appropraite changes to the node object.
3) There is a problem with the workbench_access hook_node_load() that is preventing it from altering the node object.
4) Your content type does not have workbench access enabled.
Without the workbench access id, the "contents current control section .. " condition will not function because it cannot tell if the function is associated with some workbench access id.
Now, lets assume #4 is your case.
This means that workbench access id is not enabled and therefore cannot ever match the condition.
The solution will be to test for the object property workbench_access_id and if it does not exist return FALSE.
Doing this will cause cases 1->3 to return FALSE and thus fail silently, but I suspect solving #4 is still the best way to go right now.
I will make a patch shortly.
Could you confirm if you have content that does not have "Enforce workbench access control" enabled?
#15
Here is the patch with the change mentioned in #14.
#16
I have workbench access turned on for all of my content types:
https://skitch.com/rjpeter2/8sckd/workbench-access-wb-test
Here is my rule:
https://skitch.com/rjpeter2/8sck5/editing-reaction-rule-test-wb-test
If content type == Article, then put in Workbench Access section of Education.
I edit and save a Article, and it is not put in the Education section. Seems like I'm doing it right, I was following what the guy does in this screencast: http://dev.nodeone.se/en/workbench-access (at 13:00).
#17
Try turning on the rules debugging, which is found under admin/config/workflow/rules/settings.
Either select "Log debug information to the system log " or set "Show debug information" to "Always".
This might help identify where the problem may be.
#18
That error probably depends on when Rules tries to execute, and how it changes the $node. It would also depend on which version of WA you are using. Use 7.x-1.x-dev.
#19
The errors were occurring in #13 because workbench_access_node_load() doesn't attach it's data at $node->workbench_access_id anymore, it attaches at $node->workbench_access.
#20
Re-rolled patch against latest git checkout. This patch works for the Rule I described above for me.
#21
good catch.
#22
I think
workbench_access_active_options()duplicates the existingworkbench_access_options()function.#23
I am not sure about that.
To get the list of active options, workbench access does the following all over the place:
<?php$active = workbench_access_get_active_tree();
$tree = workbench_access_get_user_tree();
workbench_access_build_tree($tree);
$options += workbench_access_options($tree, $active['active']);
?>
So, in order to call workbench_access_options() to get what is active, it seems that
those 4 lines must be used every time.
I hope I am wrong with this, I would rather have fewer lines of code and certainly less redundancy.
Perhaps the argument should be to just do those 4 lines every time and not add an additional function whose name can confuse?
Hopefully one of the worbench_access developers can clarify this issue.
#24
I agree and would then prefer to replace those lines with the new function. But that patch might need to be separated and put in before the Rules patch.
#25
I've made a separate issue for workbench_access_active_options()
#1561822: Add workbench_access_active_options()
#26
Oops, Wrong window/tab.
#27
Let's commit the other patch first, then refactor this one (if needed).
#28
Will this awesome patch be commit to HEAD soon? ; )
Thanks Kevin, drupalmonkey, Itangalo, marcus and thanks all :)
#29
Hello,
Any chance to commit this to HEAD soon?
#30
#20: workbench_access-rules_support-1285110-20.patch queued for re-testing.
#31
The last submitted patch, workbench_access-rules_support-1285110-20.patch, failed testing.
#32
Needs a re-roll. I think that using the native form support in the dev branch gets you this already.
#33
I re-rolled #20's patch.
However, I am wondering about what #22 and then @25 said.
Would it not be better to wait for #1561822: Add workbench_access_active_options() to be committed and then recreate this patch using the new function(s)?
#34
Yes, we should leverage the other patch.
This doesn't get much attention, frankly, because I never use Rules.
#35
#32, I've finally looked at the new native form support in dev branch.
In some aspects it can support the functionality added by this patch, but the rules support here adds much more than just form save actions.
These workbench_access rules allows for loading workbench access states for any kind of operation, such as during cron jobs, new account creation, or even page views.
The native forms support is just restricted to forms, so I believe rules support is still needed.
I rerolled the patch against the latest dev.
For simplicity purposes, I have applied the changes from #1561822: Add workbench_access_active_options().
Doing this is likely to make this patch fail during testing.
As soon as the said issue gets commited, I will re-test this patch.
(Drupal really needs "Blocked By" and "Blocks" issue states..)
For those testing this, be sure to apply the patch from #1561822: Add workbench_access_active_options() if it is not yet commited.
#36
I thought I found a case in the last patch where I did not, but could, use workbench_access_active_options(). However, it turns out that particular case needed both $tree and $active variables.
I am putting my change below in text, I will wait to re-roll in case more bugs are found.
diff --git a/workbench_access.module b/workbench_access.moduleindex af59524..0a5cece 100644
--- a/workbench_access.module
+++ b/workbench_access.module
@@ -1871,7 +1871,10 @@ function workbench_access_node_operations($form = array(), $form_state = array()
if (!user_access('batch update workbench access')) {
return;
}
- $options = workbench_access_active_options();
+ $active = workbench_access_get_active_tree();
+ $tree = $active['tree'];
+ workbench_access_build_tree($tree);
+ $options = workbench_access_options($tree, $active['active']);
if (empty($options)) {
return;
}
#37
The blocking patch has been committed. Patch still applies, but needs some love based on #36.
#38
I see the little correction in #36. Trying again.
#39
Actually, let me commit the minor fix first.
#40
The last submitted patch, 1285110-wa-rules.patch, failed testing.
#41
And that's corrected and removed from this patch.
Needs testers who use Rules.
#42
The last submitted patch, 1285110-wa-rules.patch, failed testing.
#43
@agentrickard: Don't give up! The test bot is just being mean.
#44
I think testbot doesn't like patches with new files in them.
#45
The problem with the patch being applied seems to be that for some reason the patch you supplied is trying to modify an existing workbench_access.rules.inc and this patch is supposed to create that file not modify it.
I have been using this patch for a while to perform accessibility validation on node save and change the workflow state to either Published or Needs Work depending on the pass/fail results of the validation check.
I came across three problems:
1) Using a rule that responds on workflow state change to call another rule that then changes the workflow state to something else gets can cause rules to not call the workflow state change response function a second time to prevent recursion.
- This is not really a problem, but it does fill the logs with warnings (and makes me feel worried).
- Nothing seems to be broken.
2) The quick and easy inline workbench_moderation change moderation state block seems to crash or do something weird while the rules module is running.
- I cannot tell if this is a problem with workbench_moderation, rules, the workbench_* rules patches I made, or some combination of the three.
- However, if I disable the inline block (found ~around line #1789 of workbench_moderation.module), then the rules never have issues.
- As far as I can tell is that the said bug is related to calling drupal_render().
3) When changing states and reacting to states using this rules code, I noticed occasional weirdness with access id's not always being properly reacted to.
- I managed to isolate the problem to how I am processing workbench_access and workbench_access_id.
- It seems that the workbench_access array is the current/previous access_id information and if, on node save, a new value is applied, there is now a workbench_access_id array with the new data. Which means I need to first test for workbench_access_id (in case this is a node_save) and then test for workbench_access.
- I created a patch, but I have since noticed that there have been quite a lot of changes to the workbench_access 7.x-1.x-dev branch. I am not certain if the stuff I used in this patch is still valid for the new code.
In summary:
#1 = design limitation or log irritation.
#2 = unknown problem with workbench_moderation + rules patches that affects workbench_access rules patches.
#3 = fixed mistake with processing workbench_access_id when available and when not available, processing workbench_access.
The attached patch contains the fix from #3 and should commit properly.
#46
Nice. Do we have documentation for how to roll patches like that? I can't find it. I was using git add -N, which is obviously wrong.
Aside from #1 and #2, is this patch de-coupled from the Workbench Moderation patch?
#47
The workbench moderation patch is completely separate as the *.rules.inc file is on a per-module/per-directory basis. There is no inter-depedency between the workbench_access and the workbench_moderation rules patches.
The actual complete _lack_ of documentation has led me to do the following:
1) git clone (project_url) some_project
2) cd some_project
3) hack..hack...slash...save
4) gid add -A
5) git diff HEAD > ../some_project-version-1.patch
6) upload!
I always felt I might be doing it wrong, but it works...
That said, I still have a lot to learn about git.
I was not aware of this -N process you used.
looking at git-add man page:
-N, --intent-to-addRecord only the fact that the path will be added later. An entry for the path is placed in the index with no content. This is useful for, among other things, showing the unstaged content of such files
with git diff and committing them with git commit -a.
It sounds like git -N might be the more proper way to generate a patch than what I am doing.
So, perhaps the following will work better:
1) git clone (project_url) some_project
2) cd some_project
3) hack..hack...slash...save
4) gid add -N
5) git diff HEAD > ../some_project-version-1.patch
6) upload!
#48
Maybe. My mistake may be that I didn't do a diff HEAD. I always do "git diff --relative" by default.