-
-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: user management feature with auth0 #562
Conversation
FeaturesUpdate
Bug Fixes
ContributorsCommit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane good call today.
I see that several checks fail the CI. Can you please clear all errors and also polish the rough edges that I experienced today. Users should not have to manually reload a page to get the expected behavior.
Thank you!
0890ca2
to
a063f8e
Compare
Codecov Report
@@ Coverage Diff @@
## master #562 +/- ##
==========================================
+ Coverage 82.11% 85.58% +3.46%
==========================================
Files 35 42 +7
Lines 973 1311 +338
Branches 84 112 +28
==========================================
+ Hits 799 1122 +323
- Misses 174 189 +15
|
d1f93b9
to
356ad9a
Compare
47d1560
to
bd85dfe
Compare
0d8b090
to
2f13a9f
Compare
…e/ambianic-ui into feature/user-management
638c998
to
f77a2d4
Compare
This pull request introduces 1 alert when merging f77a2d4 into 7d7672f - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 915218c into 7d7672f - view on LGTM.com new alerts:
|
I have made changes based on your last review. As a rule of thumb, all new test suites now end with an assertion. Also, I have renamed most of the tests to have more descriptive names. The merged jest + cypress report is now being uploaded as an artifact on each CI build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane there are still a lot of unresolved conversations. Please take a look at each and provide your comment so we can resolve them. There are currently 23 unresolved conversations in this PR. Click on Conversations to see the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane a few minor comments remain. Also the lighthouse errors needs to be fixed.
.github/workflows/main.yml
Outdated
@@ -43,6 +43,11 @@ jobs: | |||
env: | |||
CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }} | |||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |||
- name: Upload Jest coverage to Github Artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane should this name value be updated to "Upload combined test coverage report ..." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i have renamed this job.
package.json
Outdated
@@ -97,7 +103,7 @@ | |||
], | |||
"license": "Apache-2.0", | |||
"nyc": { | |||
"report-dir": "coverage", | |||
"report-dir": "cypress-coverage", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane what is the purpose of setting nyc's report-dir parameter to the cypress-coverage dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially i wanted to create seperate folders for the two reports. I have restored it back to the coverage
name.
reports/from-cypress.json
Outdated
@@ -0,0 +1,40 @@ | |||
{"/home/victory/Desktop/OSS/ambianic-ui/cypress/integration/ambianic-tests/about.spec.js": {"path":"/home/victory/Desktop/OSS/ambianic-ui/cypress/integration/ambianic-tests/about.spec.js","statementMap":{"0":{"start":{"line":2,"column":0},"end":{"line":33,"column":2}},"1":{"start":{"line":3,"column":2},"end":{"line":5,"column":4}},"2":{"start":{"line":4,"column":4},"end":{"line":4,"column":22}},"3":{"start":{"line":7,"column":2},"end":{"line":13,"column":4}},"4":{"start":{"line":8,"column":4},"end":{"line":9,"column":42}},"5":{"start":{"line":11,"column":4},"end":{"line":12,"column":57}},"6":{"start":{"line":15,"column":2},"end":{"line":20,"column":4}},"7":{"start":{"line":16,"column":14},"end":{"line":16,"column":37}},"8":{"start":{"line":17,"column":4},"end":{"line":17,"column":48}},"9":{"start":{"line":18,"column":4},"end":{"line":19,"column":54}},"10":{"start":{"line":22,"column":2},"end":{"line":27,"column":4}},"11":{"start":{"line":23,"column":14},"end":{"line":23,"column":37}},"12":{"start":{"line":24,"column":4},"end":{"line":24,"column":48}},"13":{"start":{"line":25,"column":4},"end":{"line":26,"column":49}},"14":{"start":{"line":29,"column":2},"end":{"line":32,"column":4}},"15":{"start":{"line":30,"column":4},"end":{"line":31,"column":48}}},"fnMap":{"0":{"name":"(anonymous_0)","decl":{"start":{"line":2,"column":21},"end":{"line":2,"column":22}},"loc":{"start":{"line":2,"column":27},"end":{"line":33,"column":1}},"line":2},"1":{"name":"(anonymous_1)","decl":{"start":{"line":3,"column":13},"end":{"line":3,"column":14}},"loc":{"start":{"line":3,"column":19},"end":{"line":5,"column":3}},"line":3},"2":{"name":"(anonymous_2)","decl":{"start":{"line":7,"column":33},"end":{"line":7,"column":34}},"loc":{"start":{"line":7,"column":39},"end":{"line":13,"column":3}},"line":7},"3":{"name":"(anonymous_3)","decl":{"start":{"line":15,"column":30},"end":{"line":15,"column":31}},"loc":{"start":{"line":15,"column":36},"end":{"line":20,"column":3}},"line":15},"4":{"name":"(anonymous_4)","decl":{"start":{"line":22,"column":30},"end":{"line":22,"column":31}},"loc":{"start":{"line":22,"column":36},"end":{"line":27,"column":3}},"line":22},"5":{"name":"(anonymous_5)","decl":{"start":{"line":29,"column":27},"end":{"line":29,"column":28}},"loc":{"start":{"line":29,"column":33},"end":{"line":32,"column":3}},"line":29}},"branchMap":{},"s":{"0":2,"1":2,"2":8,"3":2,"4":2,"5":2,"6":2,"7":2,"8":2,"9":2,"10":2,"11":2,"12":2,"13":2,"14":2,"15":2},"f":{"0":2,"1":8,"2":2,"3":2,"4":2,"5":2},"b":{},"_coverageSchema":"1a1c01bbd47fc00a2c39e90264f33305004495a9","hash":"d4de55937d062e0d9578189324a6e10f4d882989"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane what is the purpose of adding a coverage report to the github repo? Did you mean to add these files to .gitignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a few more unaddressed and unresolved comments.
.github/workflows/main.yml
Outdated
@@ -43,6 +43,11 @@ jobs: | |||
env: | |||
CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }} | |||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |||
- name: Upload Jest coverage to Github Artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane did you see this comment?
./coverage/coverage-final.json | ||
./coverage/jest/coverage-final.json | ||
./cypress-coverage/coverage-final.json | ||
./jest-coverage/coverage-final.json | ||
fail_ci_if_error: true # optional (default = false) | ||
path_to_write_report: ./coverage/codecov_report.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane Looking at the codecov docs again, I think we can use this file to directly upload it as a combined coverage report artifact for github action and skip all the nyc merging code in this action workflow. Worth a try. Here is some example docs to look at:
https://github.com/codecov/codecov-action#example-workflowyml-with-codecov-action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane Did you see this comment from 15 days ago?
If you looked at the out.json
file currently uploaded as a github artifact, you will notice that it is not convenient to read. Let's see if we can use a txt or html format that allows us to quickly understand the coverage report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented the upload action to upload the file uploaded from this codecov job.
Should i discard the previous copy scripts implemented manually?
If you looked at the out.json file currently uploaded as a github artifact, you will notice that it is not convenient to read. Let's see if we can use a txt or html format that allows us to quickly understand the coverage report.
The current file being uploaded to GH artifacts is in a readable txt
format.
package.json
Outdated
"codecov:e2e": "codecov -f ./coverage/coverage-final.json", | ||
"codecov:unit": "codecov -f ./coverage/jest/coverage-final.json", | ||
"codecov": "codecov -f coverage/coverage-final.json && codecov -f coverage/jest/coverage-final.json", | ||
"codecov:e2e": "codecov -f ./jest-coverage/coverage-final.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane did you see this comment ^^
@ivelin I have created this separate issue to track the Lighthouse error. I will fix it today in a new pull request and revert to this. |
The |
I have renamed the job. |
I wanted to split the two tests into separate directories. |
This pull request introduces 1 alert when merging 6cfa5a7 into 7d7672f - view on LGTM.com new alerts:
|
b0285eb
to
9b81297
Compare
This pull request introduces 1 alert when merging 9b81297 into 7d7672f - view on LGTM.com new alerts:
|
@vickywane this PR is now out of sync with upstream. Please re-sync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane see open comments.
./coverage/coverage-final.json | ||
./coverage/jest/coverage-final.json | ||
./cypress-coverage/coverage-final.json | ||
./jest-coverage/coverage-final.json | ||
fail_ci_if_error: true # optional (default = false) | ||
path_to_write_report: ./coverage/codecov_report.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane Did you see this comment from 15 days ago?
If you looked at the out.json
file currently uploaded as a github artifact, you will notice that it is not convenient to read. Let's see if we can use a txt or html format that allows us to quickly understand the coverage report.
package.json
Outdated
"codecov:e2e": "codecov -f ./coverage/coverage-final.json", | ||
"codecov:unit": "codecov -f ./coverage/jest/coverage-final.json", | ||
"codecov": "codecov -f coverage/coverage-final.json && codecov -f coverage/jest/coverage-final.json", | ||
"codecov:e2e": "codecov -f ./jest-coverage/coverage-final.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane Reminder. This comment was made 25 days ago...
"codecov:unit": "codecov -f ./coverage/jest/coverage-final.json", | ||
"codecov": "codecov -f coverage/coverage-final.json && codecov -f coverage/jest/coverage-final.json", | ||
"codecov:e2e": "codecov -f ./jest-coverage/coverage-final.json", | ||
"codecov:unit": "codecov -f ./jest-coverage/coverage-final.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vickywane codecov:unit
and codecov
appear to be stale. Are these used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. They are not being used by the application itself, except by other contributors.
I have removed the codecov:unit
script.
No they are not being used by the application. Alongside with Should i proceed to remove them? |
This pull request introduces 1 alert when merging 9680489 into 011fdad - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 159579b into 011fdad - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 901a755 into 011fdad - view on LGTM.com new alerts:
|
901a755
to
04ced60
Compare
This pull request introduces 1 alert when merging 04ced60 into 011fdad - view on LGTM.com new alerts:
|
This closes this
This PR contains work done to implement the new user management feature.
Note: This PR is an active WIP.
A test of the work being done can be found in the Staging PWA here.
This design flow diagram visualizes the steps performed by different actors to achieve a user subscription.