Comments

ankitjain28may created an issue. See original summary.

chiranjeeb2410’s picture

Assigned: Unassigned » chiranjeeb2410

Posting the current work shortly.

chiranjeeb2410’s picture

Status: Active » Needs review
StatusFileSize
new6.5 KB

Following patch contains the Affiliate Connect Config Settings from which every created plugin would intake some common config. Also includes the config schema and associated routes as well for the config form.

However, need to add the data cron storage dropdown containing additional configurations at which I'm currently at a fix. Will resolve that and add it asap.

chiranjeeb2410’s picture

StatusFileSize
new33.42 KB
new129.66 KB
new132.77 KB

Attached screenshot of the current status of config page setup. Works fine for now. Adding data storage dropdown asap.

borisson_’s picture

Status: Needs review » Needs work
+++ b/src/Form/AffiliatesConnectSettingsForm.php
@@ -0,0 +1,144 @@
+  /**
+   * {@inheritdoc}
+   */
+  // public function validateForm(array $form, FormStateInterface $form_state)
+  // {
+  //   parent::validateForm($form, $form_state);
+  // }

We should just remove this instead of commenting it.

The curly braces in other places in that class are not in the right place. Those should be fixed as well.

thedrupalkid’s picture

StatusFileSize
new37.73 KB
new28.51 KB

One suggestion there..

Please have a help and config links in place by editing info.yml file for config.

configure - If your module offers a configuration form, then you can specify the route to this form here. It will then show up as a link in the Extend page (/admin/modules) when the user expands the details.

Ref: https://www.drupal.org/docs/8/creating-custom-modules/let-drupal-8-know-...

chiranjeeb2410’s picture

@borisson and @thedrupalkid, thanks for the suggestions. Would make it right away.

chiranjeeb2410’s picture

@borrison_ and @thedrupalkid,

Uploading complete Configuration setup form including all settings as required. Also, addressed all the issues as
suggested in #5 and #6. Attaching screenshots as well. Please review.

chiranjeeb2410’s picture

Status: Needs work » Needs review
dbjpanda’s picture

+++ b/src/Form/AffiliatesConnectSettingsForm.php
@@ -0,0 +1,285 @@
+      '#options' => [
+         'A' => t('Everyday'),
+         'B' => t('Every 15 days'),
+         'C' => t('Every week'),
+         'D' => t('Every month'),
+      ],
...
+      '#options' => [
+         'A' => t('Everyday'),
+         'B' => t('Every 15 days'),
+         'C' => t('Every week'),
+         'D' => t('Every month'),
+      ],
...
+      '#options' => [
+         'A' => t('Every 30 mins'),
+         'B' => t('Every 1 hour'),
+         'C' => t('Every 2 hours'),
+         'D' => t('Every 5 hours'),
+         'E' => t('Every 10 hours'),
+         'F' => t('Every 15 hours'),
+         'G' => t('Every 20 hours'),
+      ],

1- Use $this->t()

2- Better you can replace A,B,C,D.. with actual readable keys.

dbjpanda’s picture

Status: Needs review » Needs work
ankitjain28may’s picture

  1. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +         'A' => t('Everyday'),
    

    Use $this->t() in place of t()

  2. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +      '#required' => true,
    

    Remove this line as it will ask for user input even if user has disabled the data storage.

  3. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +        "required" => [
    

    Only this is required as it will check in case the user has enabled the field (data storage)

  4. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +      '#required' => true,
    

    Same goes here

  5. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +         "required" => [
    

    Same goes here

  6. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +      '#required' => true,
    

    Same, change it too.

  7. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +         "required" => [
    

    This too.

  8. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +      '#default_value' => $config->get('Color'),
    

    It should be 'color'

  9. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,285 @@
    +      ->set('data_storage', $values['data_storage'])
    

    Save the cron form user input too as it's only saving 'data_storage' field.

chiranjeeb2410’s picture

Status: Needs work » Needs review
StatusFileSize
new14.33 KB
new148.7 KB
new143.73 KB
new13.55 KB

@borrison_, @dbjpanda, @ankitjain28may,

The updated patch contains the completed and corrected code for the Configuration Settings page with all the
related route and schema/config data along with it. I have fixed all the issues as mentioned in the latest previous
comments regarding the form.

Also, the skeleton and the config page works smooth as of now without any errors. Attaching screenshots for the same.

Please review.

ankitjain28may’s picture

Status: Needs review » Needs work
  1. +++ b/affiliates_connect.links.menu.yml
    @@ -12,3 +12,17 @@ affiliates_connect.overview:
    +# affiliates_connect.scraper_api:
    +#   title: Affiliates Scraper API settings
    +#   description: Configure scraper affiliate api settings
    +#   route_name: affiliates_connect.scraper_api
    +#   parent: affiliates_connect.admin_config
    +#   weight: 1
    

    This is not required as of now.

  2. +++ b/affiliates_connect.routing.yml
    @@ -15,3 +15,23 @@ affiliates_connect.overview:
    +# affiliates_connect.scraper_api:
    +#   path: 'admin/config/affiliates-connect/scraper-api'
    +#   defaults:
    +#     _title: 'Affiliates Scraper API settings'
    +#     _form: ''
    +#   requirements:
    +#     _permission: 'access administration pages'
    +#   options:
    +#     _admin_route: TRUE
    

    This is also not required.

  3. +++ b/config/install/affiliates_connect.settings.yml
    @@ -1,3 +1,15 @@
    +data_storage: true
    +affiliate_import:
    +scraper_import:
    +content_scrape: true
    +scrape_timer:
    

    'scrape_timer' field is missing.

  4. +++ b/config/schema/affiliates_connect.schema.yml
    @@ -2,6 +2,42 @@ affiliates_connect.settings:
    +      label: 'Allows storing product's data in your site's database'
    

    This is showing error in schema file as in between `'` is not allowed.

  5. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,291 @@
    +    $form['affiliates_connect']['data_storage_form']['affiliate_import'] = [
    ...
    +      '#default_value' => '',
    

    default should be loaded from the congif like

     #default_value' => $config->get('affiliate_import'), 
  6. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,291 @@
    +      '#options' => [
    +         '1' => $this->t('Everyday'),
    +         '2' => $this->t('Every 15 days'),
    +         '3' => $this->t('Every week'),
    +         '4' => $this->t('Every month'),
    +      ],
    

    Add default value here too.. Else it will not load the user selected field.
    And it would be better to use something like-

         '#options' => [
            'everyday' => $this->t('Everyday'),
            'every_15_days' => $this->t('Every 15 days'),
            'every_week' => $this->t('Every week'),
            'every_month' => $this->t('Every month'),
         ],
    

    So that user can read & understand the content value too. Umm, that's a suggestion, I think it should be better to ask the advice of mentors.

  7. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,291 @@
    +         '1' => $this->t('Every 30 mins'),
    +         '2' => $this->t('Every 1 hour'),
    +         '3' => $this->t('Every 2 hours'),
    +         '4' => $this->t('Every 5 hours'),
    +         '5' => $this->t('Every 10 hours'),
    +         '6' => $this->t('Every 15 hours'),
    +         '7' => $this->t('Every 20 hours'),
    

    Same goes here, Add default value here too.

  8. +++ b/src/Form/AffiliatesConnectSettingsForm.php
    @@ -0,0 +1,291 @@
    +      ->set('data_storage', $values['data_storage'])
    +      ->set('affiliate_import', $values['affiliate_import'])
    +      ->set('scraper_import', $values['scraper_import'])
    +      ->set('content_scrape', $values['content_scrape'])
    +      ->set('full_content', $values['full_content'])
    +      ->set('reviews', $values['reviews'])
    

    Save the content of 'scrape_timer' field too.

chiranjeeb2410’s picture

Status: Needs work » Needs review
StatusFileSize
new5.48 KB

@ankitjain28may,

Made all the required changes as mentioned in #14.

Please review.

chiranjeeb2410’s picture

ankitjain28may’s picture

@chiranjeeb2410 You have attached the interdiff file with the name of the patch, pls add the patch as well as interdiff correctly.

ankitjain28may’s picture

Status: Needs review » Needs work
+++ b/src/Form/AffiliatesConnectSettingsForm.php
@@ -136,13 +137,13 @@ class AffiliatesConnectSettingsForm extends ConfigFormBase {
       '#attributes' => ['class' => ['select-bbq-selector']],
       '#empty_option' => 'Select',

Add default_value field here too.

chiranjeeb2410’s picture

Status: Needs work » Needs review
StatusFileSize
new14.16 KB
new5.65 KB

Patch updated with all the requirements specified in #10, #12, #14 and #18 and attached along with
interdiff.

Up for review.

ankitjain28may’s picture

@chiranjeeb2410, Patch looks good to me, Leaving it for mentors to review.

Lal_’s picture

+++ b/src/Form/AffiliatesConnectSettingsForm.php
@@ -0,0 +1,294 @@
+
...
+
...
+

could you please add few more comments... :-)

Lal_’s picture

Status: Needs review » Needs work
chiranjeeb2410’s picture

@lal,

As in what. You need to be a bit more specific.

chiranjeeb2410’s picture

Status: Needs work » Needs review
StatusFileSize
new14.3 KB
new743 bytes

Updated patch as per suggestion in #21.

Please review.

dbjpanda’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me as well. Leaving for @borisson_ for a final review and commit.

dbjpanda’s picture

I have not tested it on local though. @ankit can you apply the patch on your local and check if it is working as expected or not .

ankitjain28may’s picture

The patch is applied successfully, storing the user content correctly and working as expected, attached images for the same.

dbjpanda’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. I have committed the patch.

chiranjeeb2410’s picture

Title: Add affiliates connect configuration form » Add affiliates connect configuration Settings
Version: » 8.x-1.x-dev
Issue summary: View changes

Status: Fixed » Closed (fixed)

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