.travis.yml fix for 7.x #255

AronNovak · 2017-04-18T04:51:58Z

Status:

rewritten YML file with upgraded PHP

Todo:

make Travis green
optionally include PHP7 in the build matrix

#133

…ript too

…rivial due to phpenv

AronNovak · 2017-04-21T11:39:05Z

Travis now realistically shows that the tests do not pass, we have identical result on local execution:

…server causes whatever/10.00/group path a 404, it does not reach Drupal. Instead of patching Drush, rewrote the test to check similar, but a bit different scenario

…vis-7.x

AronNovak · 2017-05-12T15:18:26Z

The other failing assertion is because somehow plugins/entityreference/behavior/OgBehaviorHandler.class.php / OgMembershipCrud has an issue:

      if (empty($item['state']) && $entity_type == 'user' && !og_user_access($group_type, $gid, 'subscribe without approval', $entity) && !og_user_access($group_type, $gid, 'administer group')) {
        $item['state'] = OG_STATE_PENDING;
      }

Is not effective anymore.

amitaibu · 2017-05-14T11:42:09Z

@AronNovak Can you invest 2h in this please

AronNovak · 2017-05-15T08:35:07Z

Sure.
So this particular assertion became broken after the merge of #120 , which totally make sense. Now thinking about what to test meaningfully to avoid to remove it completely.

…r has group admin permission, we should not set new membership to pending. But if he is not an admin, we exit early from the membership Crud. Adapted the test to test admin permission check of the Crud itself

AronNovak · 2017-05-15T09:18:51Z

I highly refactored that particular test, 1b32b2c - the commit message has the explanation, waiting for the green from Travis.

…piwik/piwik/pull/6963/files

…result of simpletest

AronNovak · 2017-05-17T14:55:08Z

Very good, we have the Travis green. To make sure the opposite direction works, by intention, now i break a test, then let's see if we have red again, then going to revert that change.

amitaibu · 2017-05-17T17:34:51Z

plugins/entityreference/behavior/OgBehaviorHandler.class.php

@@ -122,11 +122,7 @@ public function OgMembershipCrud($entity_type, $entity, $field, $instance, $lang
    foreach ($items as $item) {
      $gid = $item['target_id'];

-      // Must provide correct state in the event that approval is required.
-      if (empty($item['state']) && $entity_type == 'user' && !og_user_access($group_type, $gid, 'subscribe without approval', $entity) && !og_user_access($group_type, $gid, 'administer group')) {


Removing this seems wrong. I guess you tested a specific case of adding members via UI, but this is about creating membership via the field API.

I believe the existing code should be reverted

Now this is unreachable code, due to og_user_access($group_type, $gid, 'administer group'), versus https://github.com/Gizra/og/blob/7.x-2.x/plugins/entityreference/behavior/OgBehaviorHandler.class.php#L98 , that's why I removed. As far as i see, now it became impossible to trigger that branch.

If the user has group admin permission, we should not set new membership to pending. But if he is not an admin, we exit early from the membership Crud. Adapted the test to test admin permission check of the Crud itself.

And I think #120 - this issue justifies the change.

Can you please test this scenario:

Create a private group (that is with og_access module enabled and configured)

Allow in the OG permissions, to subscribe to group

As a non-admin user visit directly the subscribe URL

Try to subscribe

What is the OgMembership->state you get?

So I used the same environment as we use in Travis, same PHP built-in webserver, same setup, and I was on this branch, not the main one.
OG Access configured:

Non-member regular Drupal user wants to join:

After join, he sees this:

And in the database, the state is:

And state 2 is Pending: https://github.com/Gizra/og/blob/7.x-2.x/og.module#L26 , so it still works as expected.

amitaibu · 2017-05-17T17:35:57Z

og_ui/og_ui.test

+    // Numeric values that are not consist of decimal characters are forbidden.
+    // 0x1 for instance is equivalent to 1
+    // http://php.net/manual/en/language.types.integer.php
+    $this->drupalGet('entity_test/0x' . $this->entity->pid . '/group');


we could also just do $this->drupalGet('entity_test/666/group'); here

Sure, just added.

amitaibu · 2017-05-17T17:36:57Z

og_access/og_access.test

    $membership = og_get_membership('node', $this->group->nid, 'user', $this->user->uid);
-    $this->assertEqual($membership->state, OG_STATE_PENDING, t('User membership is pending.'));
+    $this->assertFalse(is_object($membership), t('Non-admins cannot add members to private groups.'));


the assertion isn't the same as the original one. It's not about interaction with admins, it's about a user trying to subscribe to a private group -- they should be pending by default

Yes, it isn't the same, but after https://github.com/Gizra/og/pull/124/files , it's not possible to test it this way, based on my current understanding.

…etection" This reverts commit e6a6a02.

…nse test too

…vis-7.x

amitaibu · 2017-06-23T09:50:41Z

What's the status here?

AronNovak · 2017-06-23T14:25:41Z

@amitaibu thanks a lot for the reminder, based on the last Travis output, i pushed one more commit, it's supposed to provide green status, we're very close (or ready) on this PR to achieve the goal.

AronNovak · 2017-06-23T14:46:09Z

ready for (a final) review

amitaibu · 2017-06-23T16:54:27Z

Finally, travis is green. Thank you!

AronNovak added 11 commits April 18, 2017 06:49

.travis.yml: initial attempt to fix the test logic for 7.x

608c8f7

.travis.yml: do not alter sendmail_path for site install

e90cec3

.travis.yml: download Drupal 7.x, not 8.x

6c5be09

.travis.yml: restore sendmail stubbing

7f97324

.travis.yml: assume that before_script change directory applies to sc…

08c3152

…ript too

.travis.yml: use testing script's --php version, path to php is not t…

a2be52d

…rivial due to phpenv

.travis.yml: restore local webserver

737a82c

.travis.yml: move the launch of the webserver to the proper location

bdb9a6c

.travis.yml: switch to Composer-based Drush due to drush-ops/drush#2090

e393850

.travis.yml: fix Drush path, add PHP7 to the matrix

2349059

.travis.yml: fix sendmail stubbing for newer Drush

751cd1e

AronNovak added 2 commits May 12, 2017 17:09

og_ui/og_ui.test: fixed failing access test. Drush + built-in PHP web…

72a4416

…server causes whatever/10.00/group path a 404, it does not reach Drupal. Instead of patching Drush, rewrote the test to check similar, but a bit different scenario

Merge branch '133-travis-7.x' of github.com:AronNovak/og into 133-tra…

a9c8fb8

…vis-7.x

removed unreachable code in OgBehaviorHandler.class.php . If the $use…

1b32b2c

…r has group admin permission, we should not set new membership to pending. But if he is not an admin, we exit early from the membership Crud. Adapted the test to test admin permission check of the Crud itself

AronNovak added 4 commits May 15, 2017 13:15

.travis.yml: fix mysql server has gone away, from https://github.com/…

df95e55

…piwik/piwik/pull/6963/files

.travis.yml: fix error checking

5e9973b

.travis.yml: negate the result of the egrep simply when checking the …

7083eef

…result of simpletest

.travis.yml: quote bash commands to preserve special characters

8d1bb72

og_ui/og_ui.test: temporary test break to test Travis error detection

e6a6a02

amitaibu reviewed May 17, 2017

View reviewed changes

AronNovak added 2 commits May 17, 2017 19:41

Revert "og_ui/og_ui.test: temporary test break to test Travis error d…

2ce02af

…etection" This reverts commit e6a6a02.

og_ui.test: aside 403 group admin page tests, add a regular 404 respo…

251c173

…nse test too

Merge branch '133-travis-7.x' of github.com:AronNovak/og into 133-tra…

cd2d518

…vis-7.x

og_ui: fix non-existing group test

dddfd86

AronNovak changed the title ~~WIP: .travis.yml fix for 7.x~~ .travis.yml fix for 7.x Jun 23, 2017

amitaibu merged commit e477daf into Gizra:7.x-2.x Jun 23, 2017

AronNovak deleted the 133-travis-7.x branch June 23, 2017 19:49

alesrebec mentioned this pull request Jul 11, 2017

Added option in og_context_plugin_argument_default_group_context.inc to accompany the permission rewrite in og_context(). #131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.travis.yml fix for 7.x #255

.travis.yml fix for 7.x #255

AronNovak commented Apr 18, 2017 •

edited

Loading

AronNovak commented Apr 21, 2017

AronNovak commented May 12, 2017

amitaibu commented May 14, 2017

AronNovak commented May 15, 2017

AronNovak commented May 15, 2017

AronNovak commented May 17, 2017

amitaibu May 17, 2017

AronNovak May 17, 2017 •

edited

Loading

AronNovak May 17, 2017

amitaibu May 17, 2017

AronNovak May 18, 2017

amitaibu May 17, 2017

AronNovak May 18, 2017

amitaibu May 17, 2017

AronNovak Jun 23, 2017

amitaibu commented Jun 23, 2017

AronNovak commented Jun 23, 2017

AronNovak commented Jun 23, 2017

amitaibu commented Jun 23, 2017

.travis.yml fix for 7.x #255

.travis.yml fix for 7.x #255

Conversation

AronNovak commented Apr 18, 2017 • edited Loading

AronNovak commented Apr 21, 2017

AronNovak commented May 12, 2017

amitaibu commented May 14, 2017

AronNovak commented May 15, 2017

AronNovak commented May 15, 2017

AronNovak commented May 17, 2017

Choose a reason for hiding this comment

AronNovak May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitaibu commented Jun 23, 2017

AronNovak commented Jun 23, 2017

AronNovak commented Jun 23, 2017

amitaibu commented Jun 23, 2017

AronNovak commented Apr 18, 2017 •

edited

Loading

AronNovak May 17, 2017 •

edited

Loading