Attached patch introduces configurable default owner permissions per ad type.

Users can't edit the contents of a text ad after creating one. Even not, if a user has the manage advertisements permission. This can certainly be treaten as a bug. However, after studying the code I found out that it would take more time to fix the bug than to implement this feature for all ad types at once.

It's done by attaching an option list of available ad permissions to all global settings forms of ad types. To keep this as integrated as possible, I've renamed some of the existing global settings form builder functions to a common standard (ad_[type]_global_settings).

Of course, this could be enhanced further by actually turning the global settings form into a new operation of hook_adapi(). However, since ad modules handle form values differently in form submit functions, I chose the easier way by prepending a common form submit handler for all global settings forms.

Benefit aside from that: Most hook_menu() implementations in ad_[type] modules are gone, which means less duplicate code.

CommentFileSizeAuthor
#6 ad-DRUPAL-5.default-permissions.patch6.79 KBsun
#4 ad-DRUPAL-5.default-permissions.patch6.22 KBsun
ad-DRUPAL-5.default-permissions.patch7.79 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Small addition in advance: Yes, there has been this extra condition in ad_owners_add():

-      if ($permission == ALL) {
-        $permissions = module_invoke_all('adapi', 'permissions', $node);
-        break;
-      }

However, ALL is not only undefined, but also not used anywhere else in Advertisement.

jeremy’s picture

jeremy commented
Status: Needs review » Needs work

This does not quite work. In particular, "embedded ads" are not a type of ad, they are a way for displaying ads. As such, if we apply your patch we lose access to the embedded ad configuration page. That said, I like the direction you are going with this, and the ability to configure default settings per-ad-type. (I had originally intended to allow these defaults to be set programmatically through an API, but never got around to it.)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Understood, I'll have a closer look at embedded ads and come up with a new patch shortly.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new ad-DRUPAL-5.default-permissions.patch6.22 KB

Re-rolled patch against current DRUPAL-5, omitting changes in ad_embed.module as requested in #2.

jeremy’s picture

jeremy commented
Status: Needs review » Needs work

Looks good. It should also add an update to ad.install to flush the cache_menu table, as otherwise the settings page isn't accessible.

I plan to commit this after 5.x-1.5 is released.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 5.x-1.4-1 » 5.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new ad-DRUPAL-5.default-permissions.patch6.79 KB

Added a module update to ad.install - following the improved update numbering scheme which, besides other benefits, primarily allows to determine to which module version/release an update belongs. Although only mentioned in D6's API documentation, all module developers are advised to use this scheme for all new updates.

jeremy’s picture

jeremy commented
Status: Needs review » Fixed

Finally committed, with some modifications. It's now possible to set default permissions for all ad types, not just those that define a settings page. Thanks for the patch!!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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