Support from Acquia helps fund testing for Drupal Acquia logo

Comments

max-kuzomko created an issue. See original summary.

max-kuzomko’s picture

max-kuzomko’s picture

Title: Add an availability » Add an availability to add/edit tampers to sources
Status: Active » Needs review
FileSize
25.79 KB

I have rerolled this patch:

  • Created a base form for both add/edit forms
  • Removed Feeds Tamper UI module
  • Added Edit form
  • Removed legacy

Thank you very much @ericgsmith for your help!

Please find attached the patch.

MegaChriz’s picture

Status: Needs review » Needs work

Good work. I was able to add and edit Tamper plugins for a feed type (as superuser).

A partial review:

  1. +++ b/feeds_tamper.routing.yml
    @@ -0,0 +1,25 @@
    +    _permission: 'administer feeds_tamper'
    +    _custom_access: '\Drupal\feeds_tamper\Form\TamperAddForm::checkAccess'
    ...
    +    _permission: 'administer feeds_tamper'
    +    _custom_access: '\Drupal\feeds_tamper\Form\TamperEditForm::checkAccess'
    

    I tested with an user that didn't have the administer feeds_tamper permission, but did have 'Tamper My importer feed type' and got an access denied. So the custom access check should check if the user has one of both permissions. Now both are required.

  2. When I try to edit a Tamper in the UI, there is a select element to change the plugin to add, but when I select a different plugin, the form does not change. I think when editing an existing tamper, you should not be able to change the plugin. This is also how it works in D7.
  3. In the D7 version of Feeds Tamper you were able to add an administrative description for the Tamper that you add. It would be useful to add a text field for that. I already defined the description key in feeds_tamper.schema.yml.
  4. +++ b/src/FeedTypeTamperMetaInterface.php
    @@ -49,6 +49,18 @@ interface FeedTypeTamperMetaInterface extends ObjectWithPluginCollectionInterfac
       /**
    +   * Updates the tamper plugin instance.
    +   *
    +   * @param \Drupal\tamper\TamperInterface $tamper
    +   *   The tamper plugin instance
    +   * @param array $configuration
    +   *   An array of tamper configuration.
    +   *
    +   * @return $this
    +   */
    +  public function updateTamper(TamperInterface $tamper, array $configuration);
    

    Minor: there should be a test for this newly added method in FeedTypeTamperMetaTest.

I like it that individual parts of the D7 code are removed. This way we can see better what would be left to port later on.

max-kuzomko’s picture

Hi @MegaChriz,

Thanks for the review!

I have fixed 1, 2, 4.
But faced with some issues with the point 3.
Once it is fixed - I will updated the patch.

max-kuzomko’s picture

Status: Needs work » Needs review

As discussed let's postpone description.

MegaChriz’s picture

@max
I've made start with writing up a review (found only tiny things so far) but wasn't able to finish it this week due me feeling a bit ill the last few days. Hopefully I'll be able to finish up the review by Monday.

  • MegaChriz committed 353b7d9 on 8.x-2.x authored by max-kuzomko
    Issue #2942629 by max-kuzomko, MegaChriz, ericgsmith: Add UI for adding/...

MegaChriz’s picture

Status: Needs review » Fixed
FileSize
4.25 KB

Committed #5 with a few minor changes (see interdiff).

Thanks Max!

Status: Fixed » Closed (fixed)

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