This is a follow-up for #1470530: Unpublish/Delete nodes not included in feed. Patch here requires the other patch to work.

Comments

Bobík’s picture

Issue summary:View changes
Status:Active» Postponed
Related issues:+#1470530: Unpublish/Delete nodes not included in feed
mikran’s picture

Status:Postponed» Needs review
MegaChriz’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests
  1. +++ b/plugins/FeedsUserProcessor.inc
    @@ -1,5 +1,7 @@

    +define('FEEDS_BLOCK_NON_EXISTENT', 'block');
    +
    /**
      * @file
      * FeedsUserProcessor class.

    Missing doc block.
    Also, the @file doc block should always come before constant definitions.

  2. +++ b/plugins/FeedsUserProcessor.inc
    @@ -132,6 +134,7 @@ class FeedsUserProcessor extends FeedsProcessor {
    +    $form['update_non_existent']['#options'][FEEDS_BLOCK_NON_EXISTENT] = t('Block non existent users');

    Text should be "Block non-existent users" (put a dash between "non" and "existent").

  3. +++ b/plugins/FeedsUserProcessor.inc
    @@ -231,4 +234,24 @@ class FeedsUserProcessor extends FeedsProcessor {
    +   * @param FeedsState $state

    Missing parameter description.

  4. +++ b/plugins/FeedsUserProcessor.inc
    @@ -231,4 +234,24 @@ class FeedsUserProcessor extends FeedsProcessor {
    +    // Delegate to parent if not unpublishing or option not set

    Missing period. Also, "unpublishing" doesn't fit in the context of users.

  5. Needs tests.
mikran’s picture

Status:Needs work» Needs review
StatusFileSize
new4.43 KB
PASSED: [[SimpleTest]]: [MySQL] 6,218 pass(es).
[ View ]
new4.7 KB

Thanks for review, I've fixed everything you listed and then I added a new counter for blocked users so it's similar to what is being done with unpublish action to nodes. I had more tests than the one included but those tests essentially uncovered a new bug so I'm opening a separate issue for that.

EDIT: #2339383: Items missing from feeds do not affect item hash even when action is taken based on that., please review that first.

mikran’s picture

Shane Birley’s picture

Does this companion to https://www.drupal.org/node/1470530 include the options to "delete" missing users from the import file? I also assume that the User 1 is ignored?

mikran’s picture

The options to delete users is already there. This patch only adds the new option to block user entities.

There is no special protection for "User 1" but the delete/block only happens for those records that were previously available on the feed. So you really have to trust your feed source if you are importing "User 1" so having that account deleted or blocked is not a real concern.

Shane Birley’s picture

Am I missing something? The deletion option appears to be missing entirely -- at least, in the latest alpha. Where is the option located? It used to appear in the user processor settings but it no longer is there.

Update: I updated to the latest dev and the option to delete non-existent users has returned.

Update: And good point on the UID 1.

MegaChriz’s picture

The patch in #5 looks fine to me. I only made a tiny correction to the test: near the end of the test the option "update_non_existent" was set to "block" twice. I removed the second line which did that.

I tested it manually like this:

  1. Configured importer to update existing users and block non-existent ones.
  2. Added a mapper to "status". Set it by default to "1" via the Feeds Tamper plugin "Set default value".
  3. Imported a CSV file with three users: created 3 users.
  4. Removed one line from the CSV and import that: blocked 1 user.
  5. Imported original file: updated 1 user. The user that was updated was active again because of the mapping to "status".

This patch should be evaluated by the testbot once #2339383: Items missing from feeds do not affect item hash even when action is taken based on that. is in. If then all tests pass, the issue is RTBC to me.

MegaChriz’s picture

StatusFileSize
new4.63 KB
PASSED: [[SimpleTest]]: [MySQL] 7,033 pass(es).
[ View ]

#2339383: Items missing from feeds do not affect item hash even when action is taken based on that. is in! Here is a re-upload from the patch in #9 with only the filename changed, so the testbot will pick up the patch.

Shane Birley’s picture

I think there is one issue with this importing methodology -- we are running on the assumption that the feed being imported is always consistent. In an ideal world, sure, this would be awesome. But what if the CSV is changing users all the time. I did a bit of tinkering and with multiple imports and I have been able to decimate a few sites during the import. I did twelve imports very close together removing users randomly and on each new import with the latest dev -- it killed UID 1 each time.

And, of course, with UID 1 blasted from the database and the import grinds to a halt. I associated all content on a site to UID and (as per the user settings configuration regarding what to do with content post user deletion) killed the test sites entirely.

How does one prevent UID 1 from ever being removed? To me, it just seems that if the end user is not really 100% sure on how the CSV system works, they can do some real damage. Shouldn't we think about some safe guards or am I missing something so incredibly simple that I should just give up and become an ice cream salesperson?

MegaChriz’s picture

@Shane Birley
You have a point that the end user may not realize that user 1 can get deleted by Feeds if he/she sets the "update_non_existent" option to "Delete non-existent users". mikran (in #7) also has a point that you must know what you are doing if you set that option. However, this issue is about adding the option to block previously imported users, not about deleting users (the delete option is not added by this issue, but is already part of dev). Can you create a new issue for this concern?

Shane Birley’s picture

I totally can.

Aaaaaaand, done: https://www.drupal.org/node/2485059

Shane Birley’s picture

Related issues:+#2485059: Protect user id 1