launchpad-reviewers team mailing list archive

[Ines Almeida <mp+490259@xxxxxxxxxxxxxxxxxx>]

Added a couple of comments regarding the new changes, and spent some time looking into the issue you mentioned about the dropdown `required` bool.

In general, Launchpad UI is set up so that we don't usually barely need to touch JS code, which is what led me to investigate a little further into that. I left a suggestion on how I think it should be done without having it in JS

Diff comments:

> diff --git a/lib/lp/snappy/templates/snap-edit.pt b/lib/lp/snappy/templates/snap-edit.pt
> index 5f911e5..e8649dc 100644
> --- a/lib/lp/snappy/templates/snap-edit.pt
> +++ b/lib/lp/snappy/templates/snap-edit.pt
> @@ -148,6 +154,47 @@
>        }, 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']");

Nit also missed in the last review: can you add more significant names to the `checkbox` and `select` variables? These are specifically fetch service related, so just naming it `fs_checkbox` and `fs_policy_select` or something similar is already nicer than the generic checkbox and select

> +        var select = Y.one("select[name='field.fetch_service_policy']");
> +        var container = Y.one("#fetch-service-policy-container");

This isn't used anymore, can be removed

> +
> +        if (!checkbox || !select || !container) return;
> +
> +        var descriptionNode = container.one("p.formHelp");

I think adding details to the user is great, but this is not the right place to update this. This is added automatically taking into account what's in the interface file (see ISnapEditableAttributes.fetch_service_policy the field description).

If you want to update the text, update it there as it will help both UI and API users and doesn't require extra JS

> +        if (descriptionNode) {
> +          descriptionNode.setHTML("The “strict” mode only allows certain resources and formats, and errors out in any case the restrictions are violated. The “permissive” mode works similarly, but only logs a warning when encountering any violations.");
> +        }
> +
> +        function toggleFetchPolicy() {
> +          var isChecked = checkbox.get('checked');
> +
> +          container.setStyle('display', 'block'); 

Missed in the previous review: I don't think this is needed

> +
> +          if (isChecked) {
> +            // Remove the "(nothing selected)" option which appears
> +            // because fetch_service_policy is set to 'required = False'
> +            var options = select.all("option");

If we wanted to do this work around, this should be done just once at the top of this script, not every time the toggle changes.

That being said, I spent some more time thinking about this after our chat and I found a better way to do this.

In the BaseSnapEditView, you can add the following to the `validate_widgets()` function so that the field is only required if we see that the use_fetch_service is True:
```
        if self.widgets.get("fetch_service_policy") is not None:
            super().validate_widgets(data, ["use_fetch_service"])
            self.widgets["fetch_service_policy"].context.required = data.get("use_fetch_service")
```

You can see similar logic there for other fields. So by default, you'll need to set the `required=True` for the fetch_service_policy field, but then add this so that before validation we set it to the right required bool.

You can then remove this logic from the JS code, and just do something like
```
function toggleFetchPolicy() {
          var showPolicyField = fs_checkbox.get('checked');
          fs_select.set('disabled', !showPolicyField);
}
```

> +            options.each(function(option) {
> +              if (option.get('value') == "") {
> +                option.remove();
> +              }
> +            });
> +            select.set('disabled', false);
> +          } else {
> +            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.