opened
samchrisinger
username_0
## Purpose

The S3 nodeSettings UI lagged behind other addons in both look and implementation. This PR fixes this.

## Changes

- Replace mako/jquery driven node settings with async/KO drive settings
- Update backend where needed to support frontend changes
- Fix tests broken by backend modifications

## Side Effects

We should regression test the behavior of S3's node settings.
Feature: Update S3 (node settings)
issue
created
sloria
username_1
Can you please add JS tests for the ViewModel?
comment
created
sloria
username_1
Please add views tests for the new routes.
comment
created
sloria
username_1
Pass finished. New UI is a nice improvement, and the code is definitely improved. Thanks for being thorough with JS tests. For future reference, new routes/views should always be accompanied by views tests.

Once those tests are added and the comments by Chris and I are addressed, this is good to merge. :dash:
comment
created
samchrisinger
username_0
@username_1, ready
comment
created
chrisseto
username_2
lgtm :vhs:
comment
created
samchrisinger
username_0
We should update s3 to implement ExternalProvider and AddonSerializer, but that may extend beyond the scope of this PR.
comment
created
chrisseto
username_2
@username_0 if you were going to implement that there are a couple more changes that S3 could use.
comment
created
samchrisinger
username_0
@username_2, definitely lets document those somewhere (maybe an issue?), but we should confer with PMs before doing more work here.
comment
created
chrisseto
username_2
:+1: They may exist as issues already? I'm not 100% certain.
comment
created
sloria
username_1
I don't think an ExternalProvider has been implemented for a non-OAuth addon yet. I agree that this should happer, but I think it can happen outside this PR.
comment
created
lbanner
username_3
Would they be UI/UX related -- or functionality? Specific to S3 -- or more about bring the S3 add-on in-line with other add-ons?
comment
created
samchrisinger
username_0
@username_3, the latter. Ideally this improves backend testability and code coherence.
comment
created
samchrisinger
username_0
Although @username_2 probably has more to say about this.
comment
created
chrisseto
username_2
@username_3 both. the ExternalProvider would bring it in-line with other. The other outstanding issue is that S3 still creates a new user account and requires much higher permissions than should require, which is an, hopefully, easy fix.
comment
created
sloria
username_1
Pass finished. :earth_asia:
comment
created
sloria
username_1
Pass finished. :earth_americas:
comment
created
sloria
username_1
Final fixes made during merge. :octopus:
comment