Closed (fixed)
Project:
Diff
Version:
6.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
25 Mar 2009 at 13:11 UTC
Updated:
3 Mar 2011 at 17:41 UTC
Jump to comment: Most recent file
Comments
Comment #1
moshe weitzman commentedThis is a frequent request. Due to lack of hook_menu_alter() which I won't get into now, I am unwilling to do this in D5. It will be in D6 for sure. Coming very soon.
Comment #2
moshe weitzman commentedActually, someone needs to port the "show diff tab by content type" feature from #120955: Integrate Diff into Core to the module. Help wanted.
Comment #3
markus_petrux commentedIs there any feature request or something in the diff queue for that?
By "show diff tab by content type" do you mean the new 'node_show_view_changes' option in the content type settings page, and the 'view_changes' option in the node edit form? If so, I can try the port to diff 6.x-2.x. Is that ok to install diff at g.d.o?
Comment #4
moshe weitzman commentedI mean, show the revisions tab only on certain content types. yes, there needs to be admin pref added to content type form.
i moved this issue to diff queue.
groups.drupal.org shows revsiions tab currently to editors. once this patch hits, we will start showing to everyone for wiki type.
Comment #5
markus_petrux commentedWell, I've been looking at the latest patch in #120955: Integrate Diff into Core and it seems to me it is not complete, unless I'm missing something.
I would like to try to work out this feature here, so I need to ask.
- We have 'view revision' permissions, and this is checked in node module by the access callback _node_revision_access().
- We want to add an option to the content type settings form to enable the 'View changes' button in the node edit form.
- This 'View changes' in the node edit form seems to allow previewing the diffs when editing.
Since latest patch doesn't seem to alter _node_revision_access(), I have these doubts:
- This 'View changes' in the node edit form is not affected by 'view revision' permission?
- I'm not sure how this affects the tab to view revisions. So, do we need variants of the 'view revision' permission the same way we have these variants for edit permissions, etc.? If so, I guess we need to create here a new access callback that implements this checks, right? wrong?
Edited to clarify my doubts as I was initially confused. Sorry.
Comment #6
markus_petrux commentedSorry, I'm confused because you asked to backport something that I've been unable to see implemented in the patch. :-/
...or is it as simple as adding the option to the content type settings form, and then only show the diff tab to users with 'view revision' permission AND only for the content types that have this option enabled?
If this is the case, then the menu items need to use a different access callback that checks users permissions, but also if the feature is enabled for the given content type. Right?
Comment #7
markus_petrux commentedWell, here's a patch that implements 'Enable revisions page' at content type level, and then uses a new access callback to check for this option, in addition to regular user permissions and existence of revisions for the given node.
It also introduces a hook_uninstall() so that site admins can clean up module variables when uninstalling the module.
Comment #8
moshe weitzman commentedYes thats what I had in mind. I really thought that was in the big D7 patch.
- the uninstall change does not look right. the preview changes button is unrelated, no? that button also got renamed in the big patch but thats not really relevant here.
- i never saw #prefix on a checkbox. Is that prettier?
I did a code review and all looks good. yhahn is the new maintainer here so lets wait for him to chime in or commit this.
Comment #9
markus_petrux commentedThe preview changes button is also implemented as an option by diff. And I have built the new checkbox using the same exact technique (#prefix et al).
The hook_uninstall() implementation should have been there just for the 'preview' changes setting. But now we are adding a new option, which means more variables per content type, so I thought we could cover this clean up stuff in this patch.
Comment #10
emok commentedI've tested the patch and think there is a glitch: we need to use the new access callback on all kinds of revision pages. Use it on 'node/%node/revisions/view/%/%' and 'node/%node/revisions/view/latest' which are defined in
diff_menu, plus indiff_menu_alteryou need to add the line$callbacks['node/%node/revisions/%/view']['access callback'] = 'diff_node_revision_access';.Then, I'd vote for a bit more flexibility. If I have a wiki content type and disable revision viewing for all other types, then even the administrators are constrained. I'd suggest either a new permission or reusing 'administer nodes' that overrides the restrictions set by this patch. By that I mean to change
diff_node_revision_accessinto:With these changes it works well to me.
Also, I made another version that incorporates the renaming of 'Preview changes' to 'View changes' that seemed to be agreed upon in issue 120955.
Comment #11
emok commentedSorry, I missed 'node/%node/revisions/%/revert' and 'node/%node/revisions/%/delete'.
I may also add that the rename-version of the patch is not fully compatible with the patches found in the other issue, particularly I did not rename the variables show_preview_changes_<type> to node_show_view_changes_<type>. I just changed the UI name of the button.
Comment #12
BenK commentedHi everyone,
What is the status of the latest patch (#11 by emok)? Are there plans to commit this to 6.x-2.x-dev of the main Diff module?
I, too, need this functionality.... I need users to be able to view revisions for some content types, but not view revisions for other ones.
If someone could let me know whether this patch is likely to be committed, I'd appreciate it.
Cheers,
Ben
Comment #13
kate commentedHi,
I am also very interested in the status of this issue. I would like to be able to have group members view revisions for their group wiki pages.
Thanks,
Kate
Comment #14
markus_petrux commentedBoth patches in #11 (with or without s/Preview/View) look good. Is there anything else we can do to help get this in?
Comment #15
samchok commentedsubscribe
Comment #16
gregglesThere is now a module - http://drupal.org/project/view_revisions_by_content_type - which seems to provide a similar feature. (also subscribe)
Comment #17
yhahn commentedThanks everyone, sorry for the delay.
http://drupal.org/cvs?commit=291234
http://drupal.org/cvs?commit=291236
Comment #19
jmlane commentedI was also wondering about that, greggles. Would http://drupal.org/project/view_revisions_by_content_type be depreciated with the above commit, once it makes it into a stable diff release? There isn't a 5.x version of that module, so the functionality is essentially reproduced here, but diff also includes some basic revision tools that should be standard in most Drupal sites exposing revisions.
Seems like it might be worth bringing this to Wim's attention so he can choose to redirect people looking at view_revisions_by_content_type to diff, if he thinks that is the best course of action.
Thanks for this patch, this is a great feature for diff that I look forward to using in production.
Comment #20
greggles@jmlane - seems reasonable to me. Please open an issue in the queue for view_revisions_by_content_type.
Comment #21
jyee commentedFwiw, "view revisions by content type" is significantly different because it integrates into role permissions. Diff disables revisions tabs on specific content types for everyone (except "administer nodes"). View revisions by content type allows you to disable the revisions tab on content types for specific user roles, which is far more flexible, especially if you have a number of specific content type author and editor roles.... and of course, it's handled in permissions, so it's easy to administrate it from one page.
Comment #22
jmlane commentedAh, I see that you are right, jyee. I will update #672654: Diff module duplicates module functionality to reflect this. I believe the View revisions by content type module's method of managing these permissions is superior when I think about this issue again.
Perhaps some of the contributors that worked on the D7 Diff integration and the code that was committed in this issue can comment on the advantages/disadvantages of putting this access check into a new permission per content type by individual user role?