feat: [FC-0006] add Verifiable Credentials optional feature #151
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @wowkalucky! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
mphilbrick211
added
the
needs test run
|
@wowkalucky still a draft, so let me know if we should not run the tests. There does seem to be a failure at the moment. |
e0d
removed
the
needs test run
|
@e0d yep - still in progress. Once everything is green I'll ping you. |
wowkalucky
force-pushed
the
dcc/oex-creds-476/verifiable-credentials
branch
from
a33bd93
to
f2b7990
Compare
|
@e0d open for review (we still expect minor updates for Web modal layout though - a single component update). |
wowkalucky
force-pushed
the
dcc/oex-creds-476/verifiable-credentials
branch
from
f2b7990
to
06efc3b
Compare
|
We just started a 2 week sprint yesterday so I'm adding this to the sprint starting on 6/19. |
|
Could we get screenshots for this PR? @wowkalucky |
mphilbrick211
added
the
needs test run
src/components/ProgramCertificate/test/ProgramCertificate.test.jsx
Outdated
Show resolved
Hide resolved
src/components/ProgramCertificate/test/ProgramCertificate.test.jsx
Outdated
Show resolved
Hide resolved
src/components/ProgramCertificateModal/ProgramCertificateModal.jsx
Outdated
Show resolved
Hide resolved
src/components/ProgramCertificateModal/ProgramCertificateModal.jsx
Outdated
Show resolved
Hide resolved
wowkalucky
force-pushed
the
dcc/oex-creds-476/verifiable-credentials
branch
from
06efc3b
to
f584c6f
Compare
|
@jsnwesson |
|
@wowkalucky just went through the things you changed and they look great! And thanks for the screenshots! I had some remaining things left open (ie. multiple useEffect, spelling/editing suggestions, test cleanup), let me know if you've any questions on them! |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #151 +/- ##
==========================================
+ Coverage 69.64% 70.34% +0.70%
==========================================
Files 16 27 +11
Lines 280 408 +128
Branches 65 90 +25
==========================================
+ Hits 195 287 +92
- Misses 85 120 +35
- Partials 0 1 +1
☔ View full report in Codecov by Sentry. |
e0d
removed
the
needs test run
|
@jsnwesson regarding the multiple useEffect hooks: The main idea here is a separation of concerns. Each effect is responsible for its own job and logically separated. Even though currently effects' logic is similar (identical), what if tomorrow it won't be the case (e.g. we'll need to refetch one piece of data in other manner)? |
|
@jsnwesson also I went through the PR and could find additional remaining requests - seems like everything is resolved from your list. Am I missing something? Please point if so. |
|
@wowkalucky thanks for the explanations on using multiple useEffects! The separation of concerns makes sense, thanks for linking to React's docs on that tip. I tagged you on the remaining things I noticed, which was a few spelling errors, mainly in one variable name. I'll mark this as approved if you can just take a look at those and see if they're intentional or need to be fixed. |
src/components/ProgramCertificateModal/ProgramCertificateModal.jsx
Outdated
Show resolved
Hide resolved
src/components/ProgramCertificateModal/test/ProgramCertificateModal.test.jsx
Show resolved
Hide resolved
src/components/ProgramCertificatesList/test/ProgramCertificatesList.test.jsx
Outdated
Show resolved
Hide resolved
src/components/ProgramCertificatesList/test/__factories__/programCertificatesList.factory.js
Outdated
Show resolved
Hide resolved
| download_url: null, | ||
| credential_id: 1, | ||
| program_uuid: '09876', | ||
| program_title: 'Programm title 2', |
There was a problem hiding this comment.
| program_title: 'Programm title 2', | |
| program_title: 'Program title 2', |
| const [modalIsOpen, openModal, closeModal] = useToggle(false); | ||
|
|
||
| const [ | ||
| verfifiableCredentialIssuanceData, |
There was a problem hiding this comment.
| verfifiableCredentialIssuanceData, | |
| verifiableCredentialIssuanceData, |
There was a problem hiding this comment.
@wowkalucky tagging you on the remaining pieces that I noticed, like this spelling error
src/components/ProgramCertificateModal/ProgramCertificateModal.jsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Jason Wesson <[email protected]> Co-authored-by: Jason Wesson <[email protected]> Co-authored-by: Jason Wesson <[email protected]>
wowkalucky
force-pushed
the
dcc/oex-creds-476/verifiable-credentials
branch
from
c27be3c
to
b42d0e8
Compare
wowkalucky
force-pushed
the
dcc/oex-creds-476/verifiable-credentials
branch
from
b42d0e8
to
1fda588
Compare
|
@jsnwesson please note: during the last updates I've discovered the lost feature (sorry for that (= ). |
|
@wowkalucky 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
) * feat: [FC-0006] add Verifiable Credentials optional feature --------- Co-authored-by: Jason Wesson <[email protected]>
* feat: [FC-0006] add Verifiable Credentials optional feature --------- Co-authored-by: Jason Wesson <[email protected]>
Context
This update implements an optional (hidden under the
ENABLE_VERIFIABLE_CREDENTIALSfeature flag) UI that serves the newly implemented Verifiable Credentials feature.Relates to openedx/credentials#1973
TODO:
Screenshots
Verifiable Credentials tab
Tab optionally displayed if Feature is activated.
Tab body lists all User's program certificates (only a single one on the screenshot).
Action button
By default, there is a single storage backend (wallet) is available, so, button is displayed as a simple one-action "Create" button (screenshot displays dev environment with additional "Open edX Wallet" pluggable backend for Feature onboarding).
Modal
Once "Create" button clicked (here this is the "Learner Credential Wallet" option), mobile storage backend flow starts.
Modal window renders QR code as a deeplink for wallet mobile application (then mobile app uses transferred data for verifiable credential API request).
Mobile application (Learner Credential Wallet)
Android + IOS (Android screenshots).