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


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/
    @@ -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/
    @@ -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/
    @@ -231,4 +234,24 @@ class FeedsUserProcessor extends FeedsProcessor {
    +   * @param FeedsState $state

    Missing parameter description.

  4. +++ b/plugins/
    @@ -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
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 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.