Problem/Motivation

The UserFlagType plugin provides a "Users may flag themselves" option. By default, this option is selected. Clearing the checkbox and saving the flag has no effect.

Proposed resolution

Determine why the option cannot be cleared. Write user flag tests to test the user flag type including the flag-self option.

Remaining tasks

  • Write patch.
  • Write tests.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#25 intediff-24-25.txt2.63 KBmartin107
#25 userFlagThemselves.25.patch4.41 KBmartin107
#24 userFlagThemselves.24.patch4.29 KBmartin107
#22 userFlagThemselves.22.patch5.29 KBsocketwench
#16 2409891.16.usersFlagThemselves.patch5.83 KBsocketwench
#13 2409891.13.usersFlagThemselves.patch5.38 KBsocketwench
#8 2409891.8.usersFlagThemselves.patch5.37 KBsocketwench
#8 2409891.8.usersFlagThemselves.patch5.47 KBsocketwench
#6 2409891-6.patch4.76 KBlokapujya
#6 2409891-6-test-only.patch3.26 KBlokapujya
#3 2409891-3.patch1.5 KBlokapujya
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

socketwench’s picture

socketwench CreditAttribution: socketwench commented

This might be due to the form default value not being set correctly:

'#default_value' => $this->getAccessUidSetting() ? 0 : 1

Will always default to "true", even when the underlying setting is '0'. Removing the conditional will likely be sufficient, but this highlights the fact there are no tests written for the user flag type.

joachim’s picture

joachim CreditAttribution: joachim commented

> '#default_value' => $this->getAccessUidSetting() ? 0 : 1

That seems weird anyway! The method returns you a bool, so why then set a 0 or 1?

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Active » Needs review
FileSize
2409891-3.patch1.5 KB 
<?php
$form_state->getValue(['access', 'access_uid']) 
?>

doesn't seem to work with the category. Will have to look into it more, to know why.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs review » Needs work
Issue tags: +Needs tests
lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Assigned: Unassigned » lokapujya
Issue summary: View changes
lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs work » Needs review
FileSize
2409891-6-test-only.patch3.26 KB
2409891-6.patch4.76 KB

Test for just the bug. I copied and edited doCreateFlag to create a "User Flag". The class doesn't currently need $nodeType, but may utilize it for other tests.

The last submitted patch, 6: 2409891-6-test-only.patch, failed testing.

socketwench’s picture

socketwench CreditAttribution: socketwench commented
FileSize
2409891.8.usersFlagThemselves.patch5.47 KB
2409891.8.usersFlagThemselves.patch5.37 KB

The test only tests the checkbox operation, not if the users are able to flag themselves. Here's an update.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

The last submitted patch, 8: 2409891.8.usersFlagThemselves.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2409891.8.usersFlagThemselves.patch, failed testing.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Assigned: lokapujya » Unassigned

we need to change 

<?php
$user_1->uid
?>

<?php
$user_1->id()
?>

in 2 places.

socketwench’s picture

socketwench CreditAttribution: socketwench commented
Status: Needs work » Needs review
FileSize
2409891.13.usersFlagThemselves.patch5.38 KB

Status: Needs review » Needs work

The last submitted patch, 13: 2409891.13.usersFlagThemselves.patch, failed testing.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

It looks like the flag link never gets rendered for the user entities because of the following line in flag_entity_view():

<?php
    // Only add cache key if flag link is displayed.
    if ($flag_type_plugin->showAsField() && !$display->getComponent('flag_' . $flag->id())) {
      continue;
    }
?>

While showAsField() returns true, there's no component in $display for the flag. Thus, the link never gets rendered.

socketwench’s picture

socketwench CreditAttribution: socketwench commented
Status: Needs work » Needs review
FileSize
2409891.16.usersFlagThemselves.patch5.83 KB

And getComponent() returns false because of this line in flag_entity_extra_field_info():

<?php
$extra[$flag->entity_type][$bundle_name]['display']['flag_' . $flag->id()] = [
?>

Since the user entity type has no bundle, the value of '0' is used instead of the correct bundle type: 'user'. See user_entity_extra_field_info().

Status: Needs review » Needs work

The last submitted patch, 16: 2409891.16.usersFlagThemselves.patch, failed testing.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

Hm. We're almost there, but we're not actually using the acces_uid setting on the user flag subtype anywhere other than to power a checkbox. While we could make a one-off in flag_entity_view(), that's not an expandable solution.

What could work is to allow flag subtype plugins to have a say in Flag::hasActionAccess(). That way, the plugin can allow or ignore operations as it sees fit.

A further extension to this would be to give subtypes the power to define permissions via Flag::getPermissions(), which could go down the inheritance chain to provide the basic "flag" and "unflag".

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/flag.module
@@ -128,6 +128,11 @@ function flag_entity_extra_field_info() {
     foreach ($flag->types as $bundle_name) {
+
+      if (empty($bundle_name)) {
+        $bundle_name = $flag->entity_type;
+      }

This looks a bit strange, surprised that this actually works, so we store a list with an empty entry for entity types that have no bundles?

At least the API does fall back to entity_type as the only bundle by default, so I'm not sure how you end up with this result?

If that is really expected, then we should have a method on $flag that abstracts this logic somehow?

socketwench’s picture

socketwench CreditAttribution: socketwench commented

$types and $entity_type really should be behind a accessor. Without it, FlagInterface isn't really complete. Then we can add logic for the $entity_type as the only bundle by default.

joachim’s picture

joachim CreditAttribution: joachim commented

> Then we can add logic for the $entity_type as the only bundle by default.

On D7 at least, entity_get_info() does that for us. I don't remember offhand the D8 equivalent to look it up.

socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Needs work » Needs review
FileSize
userFlagThemselves.22.patch5.29 KB

More than can't be set, doesn't work period. This fixes that, but it requires the patch from #2711093: Remove auto-set of bundles from FlagTestBase.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: Unassigned » martin107

I am looking at this now.

martin107’s picture

martin107 CreditAttribution: martin107 commented
FileSize
userFlagThemselves.24.patch4.29 KB

To easy the review process I am limiting this patch to a reroll.. after an overnight commit spree.

Looking at the patch it has had the effect as stripping out the changes that were split off into :-

#2711093: Remove auto-set of bundles from FlagTestBase

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned
FileSize
userFlagThemselves.25.patch4.41 KB
intediff-24-25.txt2.63 KB

The Review.

I think actually making use of the access_uid configuration option is a good idea. :)

The implementation fixes are good. The integration test picks up the bug +1

So I think this should go in...

Changes

My changes are minor nits and a method name change.

1) t() becomes $this->t()

I have heard the argument that $this->t() is not needed in tests. I think differently - the test are their own program and so I still want to pull t() from a well defined namespace and not global state.

2) testUserFlag was just a wrapper for doPluginUI so I removed one layer of function calls and came up with a new name that was more meaningful to me. Please feel free to object.

3) I added annotations for the test, just to ease the next reader into what is going on.

4) The rest is just trivial nits and what is picked up by phpcs.

  • socketwench authored 2fef53e on 8.x-4.x 
    Issue #2409891 by socketwench, lokapujya, martin107: Fixed option for...
socketwench’s picture

socketwench CreditAttribution: socketwench at FFW commented
Status: Needs review » Fixed

Thanks for the fixes, Martin!

Status: Fixed » Closed (fixed)

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