launchpad-reviewers team mailing list archive

[Ines Almeida <mp+490259@xxxxxxxxxxxxxxxxxx>]

Looking good, left a couple of comments

Diff comments:

> diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py
> index a65b679..d5ba5e9 100644
> --- a/lib/lp/snappy/browser/snap.py
> +++ b/lib/lp/snappy/browser/snap.py
> @@ -550,6 +552,9 @@ class ISnapEditSchema(Interface):
>      store_channels = copy_field(ISnap["store_channels"], required=True)
>  
>      use_fetch_service = copy_field(ISnap["use_fetch_service"], required=True)
> +    fetch_service_policy = copy_field(
> +        ISnap["fetch_service_policy"], required=False

I would say that it is required=True, even if we ignore it when `use_fetch_service` is false. In the DB, it always has a value, it is never None

> +    )
>  
>  
>  def log_oops(error, request):
> @@ -1075,6 +1081,8 @@ class SnapEditView(BaseSnapEditView, EnableProcessorsMixin):
>          if "project" in data:
>              project = data.pop("project")
>              self.context.setProject(project)
> +            if not data.get("use_fetch_service"):

Any reason why this "if statement" is related to the `if "project" in data` one? Shouldn't it be separate?

> +                data["fetch_service_policy"] = FetchServicePolicy.STRICT
>          super().updateContextFromData(data, context, notify_modified)
>  
>  
> diff --git a/lib/lp/snappy/templates/snap-edit.pt b/lib/lp/snappy/templates/snap-edit.pt
> index 5f911e5..bf0f36f 100644
> --- a/lib/lp/snappy/templates/snap-edit.pt
> +++ b/lib/lp/snappy/templates/snap-edit.pt
> @@ -148,6 +154,52 @@
>        }, window);
>      });
>    </script>
> +
> +  <!-- Script to enable/disable the fetch_service_policy dropdown -->
> +  <script type="text/javascript">
> +    LPJS.use('node', function(Y) {
> +      Y.on('domready', function () {
> +        var checkbox = Y.one("input[name='field.use_fetch_service']");
> +        var select = Y.one("select[name='field.fetch_service_policy']");
> +        var container = Y.one("#fetch-service-policy-container");
> +
> +        if (!checkbox || !select || !container) return;
> +
> +        if (!checkbox.get('checked')) {
> +          select.set('value', 'STRICT');
> +        }
> +
> +        var initialSelectedValue = select.get('value');
> +
> +        function toggleFetchPolicy() {
> +          var isChecked = checkbox.get('checked');
> +
> +          container.setStyle('display', 'block'); 
> +
> +          if (isChecked) {
> +
> +            // Remove the "(nothing selected)" option which appears

I'd remove this logic and change the `required=True` above

> +            // because fetch_service_policy is set to 'required = False'
> +            var options = select.all("option");
> +            options.each(function(option) {
> +              if (option.get('value') == "") {
> +                option.remove();
> +              }
> +            });
> +
> +            select.set('disabled', false);
> +            select.set('value', initialSelectedValue);

Can you explain why this `initialSelectedValue` logic is needed? We already are defaulting to STRICT, what is this changing?

> +          } else {
> +            initialSelectedValue = select.get('value');
> +            select.set('disabled', true);
> +          }
> +        }
> +
> +        toggleFetchPolicy(); // On page load
> +        checkbox.on('change', toggleFetchPolicy);
> +      });
> +    });
> +  </script>
>  </div>
>  
>  </body>


-- 
https://code.launchpad.net/~vaishnavi-asawale/launchpad/+git/launchpad-1/+merge/490259
Your team Launchpad code reviewers is requested to review the proposed merge of ~vaishnavi-asawale/launchpad/+git/launchpad-1:add-fetch-service-policy-dropdown into launchpad:master.