Skinr is still using the 6.x visibility options. These have been changed in 7.x and should be changed in Skinr too.

See screenshots.

CommentFileSizeAuthor
#26 patch_commit_59c243735462.patch30.97 KBmoonray
#24 skinr_943068_24.patch878 bytesbranana
#24 skinr_943068_24_against_61cb35f15f.patch17.54 KBbranana
#22 Screen shot 2011-03-30 at 6.13.59 AM.png70.41 KBrickmanelius
#16 skinr_943068_16.patch17.44 KBmoonray
#10 skinr-ui.js_.txt1.17 KBericduran
#9 943068-1.patch9.58 KBericduran
#5 943068.patch9.35 KBezra-g
#3 943068-cleaner.patch9.15 KBericduran
#2 943068.patch9.27 KBericduran
correct-visibility-settings.png57.61 KBjacine
current-visibility-settings.png74.44 KBjacine

Comments

ericduran’s picture

ericduran commented
Assigned: Unassigned » ericduran

working on this. Patch soon.

ericduran’s picture

ericduran commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 943068.patch9.27 KB

This definitely requires test. Also when testing this patch make sure you run update.php I alter the skinr_rules table to in order to add an extra field for the content type settings.

ericduran’s picture

ericduran commented
StatusFileSize
new 943068-cleaner.patch9.15 KB

Here's a cleaner patch. I left some unused code in there before.

Status: Needs review » Needs work

The last submitted patch, 943068-cleaner.patch, failed testing.

ezra-g’s picture

ezra-g commented
Status: Needs work » Needs review
StatusFileSize
new 943068.patch9.35 KB

The previous patches failed automated testing because they couldn't be automatically applied.

Here's a re-roll that should apply. I haven't performed a functional test on this.

ericduran’s picture

ericduran commented

Sweet, testing is on.

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs review » Needs work

Somehow this stops the Skinr settings fieldset from displaying.

I think we also need to provide JavaScript for the Vertical tabs, so that the summary shows in the tabs. We should just be able to steal code from the block module.

/**
 * Provide the summary information for the block settings vertical tabs.
 */
Drupal.behaviors.blockSettingsSummary = {
  attach: function (context) {
    // The drupalSetSummary method required for this behavior is not available
    // on the Blocks administration page, so we need to make sure this
    // behavior is processed only if drupalSetSummary is defined.
    if (typeof jQuery.fn.drupalSetSummary == 'undefined') {
      return;
    }

    $('fieldset#edit-path', context).drupalSetSummary(function (context) {
      if (!$('textarea[name="pages"]', context).val()) {
        return Drupal.t('Not restricted');
      }
      else {
        return Drupal.t('Restricted to certain pages');
      }
    });

    $('fieldset#edit-node-type', context).drupalSetSummary(function (context) {
      var vals = [];
      $('input[type="checkbox"]:checked', context).each(function () {
        vals.push($.trim($(this).next('label').text()));
      });
      if (!vals.length) {
        vals.push(Drupal.t('Not restricted'));
      }
      return vals.join(', ');
    });

    $('fieldset#edit-role', context).drupalSetSummary(function (context) {
      var vals = [];
      $('input[type="checkbox"]:checked', context).each(function () {
        vals.push($.trim($(this).next('label').text()));
      });
      if (!vals.length) {
        vals.push(Drupal.t('Not restricted'));
      }
      return vals.join(', ');
    });

    $('fieldset#edit-user', context).drupalSetSummary(function (context) {
      var $radio = $('input[name="custom"]:checked', context);
      if ($radio.val() == 0) {
        return Drupal.t('Not customizable');
      }
      else {
        return $radio.next('label').text();
      }
    });
  }
};
ericduran’s picture

ericduran commented

Yeah, after submitting the patch I realized the vertical tabs isn't being updated when we change value like in every other vertical tabs implementation. I'll submit a new patch later.

ericduran’s picture

ericduran commented
StatusFileSize
new 943068-1.patch9.58 KB

Updated patch to add the js file. JS works like the blocks js.

Not sure about the fieldset disappearing :-/

Sorry for being lazy and not setting up fakeadd.

ericduran’s picture

ericduran commented
StatusFileSize
new skinr-ui.js_.txt1.17 KB

my js file disappear.

jacine’s picture

jacine
she/her
Location New York
commented

The vertical tabs are great, but this is totally broken. I tried each of these separately, even clearing cache between applying settings to be sure and this is what I found:

  • Node types: I applied a class to sidebar_first for article nodes. I get the class on the homepage, sometimes on article nodes, sometimes on page nodes, and then nothing when logged out, even when there are no role settings applied.
  • Role: I tested the role functionality by adding a class for anonymous users to the sidebar. The class appeared whether I was logged in or not.
  • Path: I tried setting 2 paths ( and node/1). I got the class on the home page, but not node/1.

The code needs updating. This is still lots of D6 code.

jacine’s picture

jacine
she/her
Location New York
commented
Priority: Normal » Major
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Major » Normal 
+++ skinr.module	27 Oct 2010 00:06:31 -0000
@@ -336,10 +337,25 @@ function skinr_rule_visible($rid) {
+      if ((arg(0) != 'node' || (arg(0) == 'node' && (arg(1) == 'add' || arg(1) == 'edit')))) {

This line is not copied from Block module... is it?

arg() is out.

Powered by Dreditor.

ericduran’s picture

ericduran commented

@sun, I do not remember. But it does not look like is there now.

moonray’s picture

moonray commented

Looks like that line comes (sort of) from node_block_list_alter() in node.module.
EDIT: What do you mean with "arg() is out." exactly?

moonray’s picture

moonray commented
Assigned: ericduran » Unassigned
Status: Needs work » Needs review
StatusFileSize
new skinr_943068_16.patch17.44 KB

In the meantime, here's a working patch. All code in skinr_rule_is_visible() was lifted from node.module and block.module (including the arg() stuff).

EDIT: You'll need to manually add the new field to your DB or reinstall skinr to properly test this.
EDIT 2: Ran tests locally. All tests passed.

rickmanelius’s picture

rickmanelius commented

What can I do to test? Role and/or page? Or is it good already?

jacine’s picture

jacine
she/her
Location New York
commented

@rickman You can do the steps I did in #11 and see if all is well. I have not been able to test @moonray's patch yet, but that's what I would end up doing because last time I checked none of that was working properly. I'd make sure it passes tests locally. To do this, you turn on the Simpletest module, go to the test page, and select only Skinr's tests and run them.

rickmanelius’s picture

rickmanelius commented
Status: Needs review » Needs work

My tests show only 2 of the 3 items shown in #11 work. Filtering by content type still doesn't work totally.

Content type + region fails (displays on page and article even though article is the only one checked)
Roles work (set for admin only and not logged in didn't see it)
Urls work (node/1 was set and it showed up there, but node/2 didn't have it as expected)

As for simpletest, I've never used it. But having installed and run the entire set of tests, I didn't see an error that specifically noted 'skinr'. If there is some way to just run the skinr.test file, let me know and i'll take care of it. In the documentation and in the area to run the tests, I was unable to specify or see any results specifically noting skinr.

jacine’s picture

jacine
she/her
Location New York
commented

Thank you @rickman! Your review is much appreciated ;)

As far as simpletest goes, you definitely don't need to run all of the tests. That takes ages! If you go to admin/config/development/testing, you'll see a bunch of checkboxes. What you need to do is check the "skinr" checkbox, and then click "Run tests." This will run only Skinr's tests and will let you know whether they pass or fail. If the tests fail, then the patch likely needs work.

moonray’s picture

moonray commented

Finally some testers! :-)
Will look at the issues you found as soon as I get some time.

rickmanelius’s picture

rickmanelius commented
StatusFileSize
new Screen shot 2011-03-30 at 6.13.59 AM.png70.41 KB

@jacine
No problem. This is a great module and my team and I are planning on using it for future clients and are thus investing our time and energy now to learn it (and help develop it) so that it can be ready for prime time for our next client!

Re:simpletest. I wonder if there is an issue with that module itself because I only see the following (attached screenshot). That is why I'm not sure if it's correctly running the skinr test file. I know this isn't the proper queue to address that, but thus far, I'm unable to get a skinr check box to appear in the simpletest area (even having uninstalled simpletest and skinr/skinr ui).

I'm running 7.x-2.x-dev (March 4) skinr and simpletest 7.x-2.0

branana’s picture

branana commented

What version is #16 supposed to apply against?

branbot:skinr bran$ git apply skinr_943068_16.patch
fatal: git diff header lacks filename information when removing 1 leading pathname components (line 70)

branana’s picture

branana commented
StatusFileSize
new skinr_943068_24_against_61cb35f15f.patch17.54 KB
new skinr_943068_24.patch878 bytes

Content type + region fails (displays on page and article even though article is the only one checked)

Drupal 7 checkbox values are stored different, the "checked" items and "unchecked" items both get indexed now.

first patch is on top of the patch in comment #16
the second patch is against the HEAD (master)

branana’s picture

branana commented
Status: Needs work » Needs review
moonray’s picture

moonray commented
StatusFileSize
new patch_commit_59c243735462.patch30.97 KB

I decided to write a bunch of tests to automate testing of rules functionality. As a result I uncovered several bugs, which I then fixed. I've also added validation for saving rules to avoid potential issues.

See full list of changes at https://github.com/moonray/skinr/pull/4/files

Regular patch was attached as well.

nomonstersinme’s picture

nomonstersinme commented
Status: Needs review » Needs work

This patch seems to work but I get an undefined property notice after creating a rule that has a visibility setting to show only on the front page:

Notice: Undefined property: stdClass::$node_types in skinr_rule_load_multiple() (line 293 of /Users/admin/Sites/drupal7/sites/all/modules/skinr/skinr.module).

looked at the code here and its way above my head.. :/

moonray’s picture

moonray commented
Status: Needs work » Needs review

You'll need to disable, uninstall, then install skinr to test this patch properly (there were some DB changes). If you don't, you'll indeed get the above notice.

moonray’s picture

moonray commented
Priority: Normal » Major

Upgrading this issue to major.

Hari’s picture

Hari commented

Seems to be a minor mismatch in the patch but it looks like a non issue. Lines 94 and 95 patch call for

-        'description' => 'Contents of the "Pages" block; contains either a list of paths on which to include/exclude the region or PHP code, depending on the visibility setting.',
+        'description' => 'Contains either a list of paths on which to include/exclude the rule or PHP code, depending on "visibility" setting.',

Latest version of head currently has line 95 in skinr.test as
'description' => 'Tests Skinr API functionality.',
Anyways shouldn't be an issue I don't think except that it threw me an error when patching.

Hari’s picture

Hari commented

Was in the middle of trying to create a rule when I decided I didn't want to make that rule and would rather start over. Trying to delete yielded the following message.

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.skinr' doesn't exist: DELETE FROM {skinr} WHERE (module = :db_condition_placeholder_0) AND (element = :db_condition_placeholder_1) ; Array ( [:db_condition_placeholder_0] => page [:db_condition_placeholder_1] => 1 ) in skinr_rule_delete() (line 332 of /Users/Hari/Sites/drupal-7.0/sites/all/modules/skinr/skinr.module).

Not critical but worth mentioning so that it can be accounted for.

moonray’s picture

moonray commented
Status: Needs review » Fixed

#31 is a different (though related) issue. It's a one line fix that I've committed as well (which should have been part of a previous patch).

Again, moving forward by committing this patch. If there are any problems, post a bug report. :)
Note: this requires a re-installation of Skinr due to more DB changes.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

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

Issue tags: +Needs tests

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.