Posted by smk-ka on May 22, 2009 at 5:05pm
| Project: | Feedback |
| Version: | 7.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Sometimes it is desirable to have feedback show up only on selected pages, and sometimes the exact opposite, ie. not on certain pages. Thus, we should implement a blacklist/whitelist of URLs similar to the one on the block configuration page.
Comments
#1
How about implementing optional support for http://drupal.org/project/visibility_api ? (no D6 release yet though)
#2
It would appear I can configure the feedback block visibility through the normal block config (followed the block edit links provided in the zen theme.) However, the configurations there are not honored. I imagine the block is show by other means... perhaps that function that shows content in regions programmatically?
Would be rad if it borrowed the normal block rendering... however I know that the javaScript caching can get screwed up when dealing with jquery blocks from js files.
#3
+1
#4
http://drupal.org/project/getsatisfaction has an admin interface to enable/disable the feedback to the correct pages.
#5
I would also love to see visibility_api integrated.
I can confirm that the current bock visibility does not get honored.
--It does big time.
#6
This is related to #327823: Prevent duplicate feedback forms when placing feedback block, in that if we had a viable option to use feedback as a regular block then normal block visibility could be used.
#7
Why not just honour the block's visibility settings?
Isn't that what people expect a block to do?
Even if it does end up supporting visibility_api or anything else it probably should honour those normal block visibility settings because it is a block.
#8
#1130030: Option to exclude feedback on certain pages closed as duplicate
#9
#1144446: Feedback block shows up in embedded media lightbox closed as duplicate
I think we need to just add some path-based visibility settings. Visibility API may not get ported to 7 and it doesn't seem worth the dependency.
#10
Patch adds a setting for paths to exclude, similar to block configuration settings.
#11
+++ feedback.admin.inc@@ -128,14 +128,16 @@ function feedback_admin_view_form_submit($form, &$form_state) {
+ $form['feedback_excluded_paths'] = array(
+ '#title' => t('Paths to exclude from feedback display'),
+ '#description' => t("Specify pages by using their paths. Enter one path per line. The '*' character is a wildcard. Example paths are %blog for the blog page and %blog-wildcard for every personal blog. %front is the front page.", array('%blog' => 'blog', '%blog-wildcard' => 'blog/*', '%front' => '<front>')),
+ '#type' => 'textarea',
+ '#default_value' => variable_get('feedback_excluded_paths', 'admin/reports/feedback'),
Minor code style remark, otherwise looks cool:
Ideally, the #type property should always come first, then #title, then stuff like #default_value, #required, etc, and only lastly, least importantly (in terms of code and form structure), the #description (which usually happens to be lengthier, so more important properties might possibly be "hidden", depending on your code editor and editor configuration).
Powered by Dreditor.
#12
mmm, though... tests? Seems to be easy to test.
#13
Switched the form ordering and added a test.
#14
+++ b/feedback.module
@@ -162,7 +162,7 @@ function feedback_block_view($delta = '') {
+ if (user_access('access feedback form') && !feedback_exclude_path()) {
@@ -181,6 +181,28 @@ function feedback_page_build(&$page) {
+function feedback_exclude_path() {
...
+ $pages = drupal_strtolower(variable_get('feedback_excluded_paths', 'admin/reports/feedback'));
ok, some last tweaks:
I don't find that function name to be overly descriptive. A name of feedback_match_path() would be more clear in terms of what the function is doing and what it is for.
But of course, feedback_match_path() doesn't really work on its own. Which brings me to my second proposal: Let's move the variable_get() to the calling code and add a $paths argument to the function.
With these changes, the purpose of calling the function is crystal clear:
feedback_match_path($paths);or
feedback_match_path(variable_get('feedback_paths_excluded', 'admin/reports/feedback');
Also simplifies the phpDoc summary: "Returns whether $paths matches the current path or URL alias."
+++ b/tests/feedback.test@@ -52,4 +52,19 @@ class FeedbackTestCase extends DrupalWebTestCase {
+ function testFeedbackVisibility() {
+ $this->drupalLogin($this->admin_user);
+ $edit = array(
+ 'feedback_excluded_paths' => 'user*',
+ );
+ $this->drupalPost('admin/config/user-interface/feedback', $edit, t('Save configuration'));
+ $this->drupalGet('user');
+ $this->assertNoRaw('<span class="feedback-link">' . t('Feedback') . '</span>', t('Feedback form not shown.'));
+ $this->drupalGet('node');
+ $this->assertRaw('<span class="feedback-link">' . t('Feedback') . '</span>', t('Feedback form shown.'));
+ }
To make this a valid unit test, we need to replace 'node' in the last two lines with 'user' and move them to the top.
I.e.: 1 path, 1 setting, 1 expected difference.
Powered by Dreditor.
#15
Made the changes above. I made feedback_match_path more like drupal_match_path's definition.
#16
yay, thanks! :)
#17
#18
Automatically closed -- issue fixed for 2 weeks with no activity.