cloudigrade / frontigrade

GNU General Public License v3.0
4 stars 1 forks source link

13 account modal wizard #41

Closed cdcabrera closed 6 years ago

cdcabrera commented 6 years ago

What's in this PR

What's NOT in this PR

Updated Demos

jul-05-2018 22-53-07

jul-05-2018 22-49-16

Prior Demos

jul-03-2018 03-42-08

Pending state after submitting the new account

screen shot 2018-07-03 at 1 15 18 pm

Pending cancel

jul-03-2018 13-16-29

"Current" Error state on API error

screen shot 2018-07-03 at 1 12 38 pm

updates #13

codecov[bot] commented 6 years ago

Codecov Report

Merging #41 into master will decrease coverage by 1.56%. The diff coverage is 63.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   76.72%   75.16%   -1.57%     
==========================================
  Files          37       39       +2     
  Lines         391      451      +60     
  Branches       56       67      +11     
==========================================
+ Hits          300      339      +39     
- Misses         82       97      +15     
- Partials        9       15       +6
Impacted Files Coverage Δ
...components/accountWizard/accountWizardConstants.js 100% <ø> (ø) :arrow_up:
src/components/formField/formField.js 100% <ø> (ø) :arrow_up:
...mponents/accountWizard/accountWizardStepResults.js 100% <100%> (ø)
src/redux/actions/accountActions.js 100% <100%> (ø) :arrow_up:
src/constants/apiConstants.js 100% <100%> (ø)
src/redux/constants/accountConstants.js 100% <100%> (ø) :arrow_up:
src/services/accountServices.js 100% <100%> (ø) :arrow_up:
.../components/accountWizard/accountWizardStepRole.js 88.88% <100%> (ø) :arrow_up:
src/components/accountWizard/accountWizard.js 39.13% <26.31%> (-9.26%) :arrow_down:
...c/components/accountWizard/accountWizardStepArn.js 55.55% <55.55%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 971e717...3f70663. Read the comment docs.

elyezer commented 6 years ago

form field validation for the ARN, regex pattern still required

Is there an issue created for this already?

API endpoint for the ARN post... a mock endpoint was created instead to complete the work.

Same here.

Let's make sure we have open issues for those so we can get this merged and move on, what do you think?

elyezer commented 6 years ago

I was trying to verify the related issues AC and found that when trying to create with a valid ARN the wizard gives an error and I see that some information is missing from the POST request:

resourcetype: "This field is required"

After getting the above error I closed the wizard and started it again. Going to the second step Role the Cloudigrade account id text field is empty and clicking on copy does not copy anything. Check the screen shot below:

screenshot from 2018-07-04 12-11-07

Is the above issues something that this PR should take care of? I can't verify more items because some of them are TBD and getting the error I can't validate that the cloud account was actually saved.

cdcabrera commented 6 years ago

@elyezer can do on the opening of issues for the ARN regex and API endpoint.

Step 2 however, based on the screenshot, looks like it didn't fire the API call again, taking a look, good catch!

cdcabrera commented 6 years ago

@elyezer @kdelee regarding Step 2 not loading the API call for the account ID. There looks like there may be inconsistent behavior around how the API responds to the UI, had trouble telling whether that was a testing environment issue or an actual problem.

Reason for not thinking its the UI service call:

Possible solutions:

@cloudigrade/engineering

kdelee commented 6 years ago

Woop!

screenshot from 2018-07-06 13-26-16

and confirmed it is on the server with api client from integrade:

In [2]: from integrade import api

In [3]: client = api.Client()

In [4]: client.get('/api/v1/account/')
Out[4]: <Response [200]>

In [5]: client.get('/api/v1/account/').json()
Out[5]: 
{'count': 1,
 'next': None,
 'previous': None,
 'results': [{'account_arn': 'arn:aws:iam::311230538223:role/role-for-cloudigrade',
   'aws_account_id': '311230538223',
   'created_at': '2018-07-06T17:26:12.249630Z',
   'id': 8,
   'name': 'my dev08 account',
   'updated_at': '2018-07-06T17:26:12.249661Z',
   'url': 'http://test.cloudigra.de/api/v1/account/8/',
   'user_id': 16,
   'resourcetype': 'AwsAccount'}]}

Only thing that was a bit not-pretty was text in 502 error. Granted we need to figure out how to get rid of all these 502/503/504 errors but this is not so good looking: screenshot from 2018-07-06 13-24-51

You could write and issue and follow up with 500 error at another time because I think there is other error handling decisions to be made.

cdcabrera commented 6 years ago

awesome @kdelee ! and can do on placing a nicer 500 level check in there as a follow up

elyezer commented 6 years ago

awesome @kdelee ! and can do on placing a nicer 500 level check in there as a follow up

@cdcabrera I can haz an issue?

elyezer commented 6 years ago

Maybe that can be handled when working on https://github.com/cloudigrade/frontigrade/issues/43. If having a better 5XX handler does not make sense to be on that issue feel free to remove the bits related to it on the issue.