FEATURE: Improve group email settings UI (PR #13083)

Still very rough, do not review. I may split this into smaller pull requests.

GitHub

I am not sure if this will come back to bite me…I think if all the acceptance tests pass it should be OK

Well they ran okay! As a little more context I wanted to test a bad response here:

But if I just did that the actual error was thrown and stopped the tests, even though the popup shows the message :confused:

This looks weird. Why do you need this initializing field?

This looks good to me. Most of the comments I left are related to code readability and maintainability. :+1:

I was not able to test functionality because I lost access to my Gmail burner account. :cry:

Why don’t you set the values directly, with a single setProperties?

        this.form.setProperties({
          imap_server: "imap.gmail.com",
          imap_port: 993,
          imap_ssl: true
        });

I think we prefer to use const when possible.

Would it be better UI to show an error immediately and disable Submit button, as we do for example in the sign up form?

        this.setProperties({
          "group.imapSettingsValid": true,
          "group.imap_server": this.form.imap_server,
          "group.imap_port": this.form.imap_port,
          "group.imap_ssl": this.form.imap_ssl,
        });

or (as I prefer):

        this.group.setProperties({
          imapSettingsValid: true,
          imap_server: this.form.imap_server,
          imap_port: this.form.imap_port,
          imap_ssl: this.form.imap_ssl,
        });

There is no need to set this to null because it is a component argument.

Why is it necessary do copy these settings? I assume because you do not want to modify the Group object until the server was updated too. In this case, I find a buffered proxy more appropriate.

Nitpick: I am a fan of oneliners. :stuck_out_tongue_winking_eye:

    return mailboxesSize === 0 || !isEmpty(mailboxName);

There is no need to set this to null because it is a component argument.

I find it a bit odd that you store some temporary state in the Group object. Isn’t this internal component state?

The UI is a bit strange here because checking and unchecking the “Enable SMTP” checkbox will still result in a bootbox.

I find these two fields a bit strange. I understand they are used to keep in sync settings for IMAP and SMTP as in disable SMTP, also disables IMAP. It looks like a lot of work and prone to errors.

I am wondering if it would be better to let the server do the heavy lifting (it already does) and make it return an updated group state which would replace the local state.

EDIT: I see below you have an afterSave that seems to be doing almost exactly this, but with an additional request. In this case, I guess you might have already everything in place? :thinking:

I think the same comment from smtpEnabledChange applies here too, but I did not test.

Same comment about using a buffered proxy can be applied to this component too. Simply use a buffered proxy to hold the “dirty” state of the object and commit the changes to the object after the request to the server succeeded (or replace object entirely with server’s response and reset the buffer).

Nitpick: I find the naming of this property strange “it is REALLY disabled if it is disabled or saving”. Unfortunately, I am guilty of doing this in the past too. :man_facepalming:

I am not sure if @jjaffeux would agree with me here, but I’d suggest to simply remove this property and use (or disabled saving) in the component.

This components look almost identical with group-imap-email-settings and so all the comments aply here too. Is there anything you could do to extract the common functionality in a mixin perhaps? I guess the prefilling functionality could be extracted to a different component, but it might be more trouble than it’s worth.