Skip to content
This repository has been archived by the owner on Aug 18, 2024. It is now read-only.

Remove OG complex widget #181

Closed
wants to merge 154 commits into from

Conversation

amitaibu
Copy link
Member

@amitaibu amitaibu commented Sep 15, 2016

#160

PR also makes sure a group member can create content if they have create access.

foreach ($user_groups as $delta => $group) {
foreach ($this->getUserGroups() as $group) {
// Check user has "create" permission on this entity.
if ($og_access->userAccess($group, "create $entity_type_id $bundle", $user)->isAllowed()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pfrenssen If I remember correctly \Drupal\og\EventSubscriber\OgEventSubscriber::provideDefaultNodePermissions overrides the permissions for nodes so they will be in the format of create foo content. However that will mean we'll have to think about it everywhere.

So maybe we should remove it, and also for node follow the create [entity-type] [bundle] pattern. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amitaibu,

This seems to be a critical issue atm. Can we revert to the format create foo content here and refactor to create [entity-type] [bundle] in a follow up issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective it seems fine, but need to get approval from @pfrenssen based on above comment - #181 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already solved for group content but apparently not for groups.

In order to avoid the problems that arise from using this kind of string based permissions we have provided a flexible permission system that tracks operations and group content entity types. The idea behind this is that in order to discover the right permission to use we should not be messing with constructing strings but discover PermissionInterface objects that map to entity types and operations.

You can check GroupContentOperationPermission to see how we solved this for group content. If you want to know whether a user has permission to access the "create" operation on a group content entity you can call OgAccess::userAccessGroupContentEntityOperation() and pass the 'create' operation for this content type.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pfrenssen, thanks for the update. I assume this would work:

Suggested change
if ($og_access->userAccess($group, "create $entity_type_id $bundle", $user)->isAllowed()) {
if ($this->ogAccess->userAccessGroupContentEntityOperation('create', $group, $entity, $this->currentUser)->isAllowed()) {

@amitaibu amitaibu changed the title WIP: Remove complex widget Remove complex widget Sep 16, 2016
@amitaibu amitaibu changed the title Remove complex widget Remove OG complex widget Sep 16, 2016
@RoySegall

This comment has been minimized.

@RoySegall
Copy link
Contributor

@amitaibu Iv'e started to mess with it and I want to verify the scenarios:
Non member - can't create a group content
Member, Group owner - Can create posts in the gorup
Adminster group - Can create content in all groups? If so then why we need the adminster permissions to display the results. Even for normal members?

@RoySegall

This comment has been minimized.

src/Og.php Outdated Show resolved Hide resolved
@amitaibu

This comment has been minimized.

/**
* Tests adding groups, and node access.
*/
public function testFields() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment and method name is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry it's not wrong - just needs a little better explanation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lastly, we also need to verify non-member can't access the node/add/group_content

NodeType::create(['type' => 'group'])->save();
NodeType::create(['type' => 'group_content'])->save();

// Setting up groups and group content relations.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong comment

@amitaibu
Copy link
Member Author

amitaibu commented Oct 4, 2016

Note to self, the test should be more exhaustive and should cover:

  • Audience fields is required/ optional
  • User has/ doesn't have site-wide create group_content content permission

src/Og.php Outdated Show resolved Hide resolved
@MPParsley

This comment has been minimized.

@amitaibu

This comment has been minimized.

@MPParsley

This comment has been minimized.

@amitaibu

This comment has been minimized.

@damienmckenna
Copy link
Contributor

I did a fork of Amitai's PR to do a new one that includes the latest changes from the 8.x-1.x branch: #558

@MPParsley
Copy link
Collaborator

Closing this PR in favor of #570 (which is from a branch on this repository). Relevant remarks have been moved there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants