PLT-6763 FE: Auditors sign certificates #8
Conversation
WalkthroughThis pull request introduces a new certification form component with dynamic field handling, and integrates it into the existing Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Files selected for processing (15)
- src/components/CertificationForm/CertificationForm.tsx (1 hunks)
- src/components/CreateCertificate/CreateCertificate.scss (1 hunks)
- src/components/CreateCertificate/CreateCertificate.tsx (4 hunks)
- src/components/DAPPScript/DAPPScript.scss (1 hunks)
- src/components/DAPPScript/DAPPScript.tsx (1 hunks)
- src/components/Modal/Modal.tsx (1 hunks)
- src/components/TextArea/TextArea.scss (1 hunks)
- src/pages/auditing/reportUpload/ReportUpload.scss (1 hunks)
- src/pages/auditing/reportUpload/ReportUpload.tsx (4 hunks)
- src/pages/auditing/reportUpload/config.ts (1 hunks)
- src/pages/certification/components/certificationMetadata/CertificationMetadata.scss (1 hunks)
- src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx (1 hunks)
- src/pages/certification/components/certificationMetadata/certificationMetadata.interface.tsx (1 hunks)
- src/pages/certification/components/certificationMetadata/certificationMetadata.schema.tsx (1 hunks)
- src/pages/certification/components/certificationMetadata/config.ts (1 hunks)
Files skipped from review due to trivial changes (10)
- src/components/CreateCertificate/CreateCertificate.scss
- src/components/DAPPScript/DAPPScript.scss
- src/components/TextArea/TextArea.scss
- src/pages/auditing/reportUpload/ReportUpload.scss
- src/pages/auditing/reportUpload/ReportUpload.tsx
- src/pages/auditing/reportUpload/config.ts
- src/pages/certification/components/certificationMetadata/CertificationMetadata.scss
- src/pages/certification/components/certificationMetadata/certificationMetadata.interface.tsx
- src/pages/certification/components/certificationMetadata/certificationMetadata.schema.tsx
- src/pages/certification/components/certificationMetadata/config.ts
Additional comments (Suppressed): 9
src/components/Modal/Modal.tsx (2)
49-55: The addition of optional props
className,fullWidth, andfullScreento theDialogcomponent enhances its customization. Ensure that these new props are used correctly throughout the codebase.56-57: No issues found, the title rendering remains unchanged.
src/components/DAPPScript/DAPPScript.tsx (4)
17-19: The
DAPPScriptPropsinterface has been extended to includefieldArrayNameandconfig. Ensure that all instances where theDAPPScriptcomponent is used have been updated to pass these new props.21-72: The changes in this hunk are a significant improvement over the previous implementation. The use of
config.scriptFieldsandconfig.smartContractInfoto dynamically generate form fields makes the component more flexible and easier to maintain. This approach adheres to the DRY (Don't Repeat Yourself) principle, reducing redundancy in the code.60-68: The
registerfunction fromreact-hook-formis being spread into theInputcomponent props. This is a good practice as it allows the form to automatically track the value of the input field and handle form submission.36-43: The "Remove Script" button now only appears when there is more than one script in the form. This is a sensible change as it prevents users from removing the last remaining script, which could lead to an empty form state.
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx (1)
- 20-23: The
uuidprop is defaulted to an empty string. If theuuidis not provided, it could lead to unexpected behavior when used in thefetchData.postcall on line 94. Ensure that theuuidis always provided or handle the case where it might be undefined.src/components/CreateCertificate/CreateCertificate.tsx (1)
- 114-120: The
fetchRunDetailsfunction is called after a timeout of 1000ms. However, this function is not defined within the provided code context. Please ensure that this function is properly defined and handles potential errors.src/components/CertificationForm/CertificationForm.tsx (1)
- 192-197: Ensure that the
onSubmitfunction passed to theFormcomponent
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Outdated
Show resolved
Hide resolved
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Outdated
Show resolved
Hide resolved
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Files selected for processing (3)
- src/components/CertificationForm/CertificationForm.tsx (1 hunks)
- src/components/CreateCertificate/CreateCertificate.tsx (4 hunks)
- src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx (1 hunks)
Additional comments (Suppressed): 2
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx (1)
- 20-23: The
uuidprop is defaulted to an empty string. However, it's used in the API call at line 93. Ifuuidis not provided, the API endpoint will be incorrect and likely cause a failure. Ensure thatuuidis always provided when using this component or handle the case where it might not be.src/components/CreateCertificate/CreateCertificate.tsx (1)
- 162-193: This part of the code has become quite complex due to conditional rendering. Consider breaking down the JSX into smaller, more manageable components or functions for better readability and maintainability.
- {subscribedFeatures?.indexOf("l2-upload-report") === -1 ? <> - {certified || disableCertify ? null : (<Button - displayStyle="gradient" - onClick={() => triggerGetCertificate()} - buttonLabel={"Purchase a Certificate" + (certificationPrice ? " (" + (certificationPrice / 1000000).toString() + " ADA)" : "")} - showLoader={certifying} - />)} - {transactionId ? ( - <Modal open={openModal} title="Certification Successful" onCloseModal={onCloseModal}> - <span> - View your certification broadcasted on-chain - <a target="_blank" rel="noreferrer" href={`https://preprod.cardanoscan.io/transaction/${transactionId}`}>here</a>! - </span> - </Modal> - ) : null} - </> : <> - <Button - displayStyle="gradient" - onClick={() => setMetadataModalStatus(true)} - buttonLabel={"Review Certification Metadata"} - showLoader={certifying} - /> - <Modal - open={openMetadataModal} - title="Certification Metadata" - className="certification-metadata" - onCloseModal={()=> {}} - fullWidth - > - <CertificationMetadata uuid={uuid} onCloseForm={onCloseMetadataForm}/> - </Modal> - </>} + const PurchaseButton = () => { + if (certified || disableCertify) return null; + return ( + <Button + displayStyle="gradient" + onClick={() => triggerGet </blockquote></details></blockquote></details> </details>
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Outdated
Show resolved
Hide resolved
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Outdated
Show resolved
Hide resolved
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Outdated
Show resolved
Hide resolved
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (1)
- src/components/CreateCertificate/CreateCertificate.tsx (4 hunks)
Additional comments (Suppressed): 1
src/components/CreateCertificate/CreateCertificate.tsx (1)
- 43-57: The new state variable
openMetadataModalis introduced to handle the opening and closing of a metadata modal. This is a good practice as it allows for better control over the modal's visibility in the component. However, ensure that all instances where this modal should be closed are correctly setting this state to false.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files selected for processing (2)
- src/components/CreateCertificate/CreateCertificate.tsx (4 hunks)
- src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx (1 hunks)
Additional comments (Suppressed): 4
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx (1)
- 20-23: The
uuidprop is defaulted to an empty string. However, it's used in the API call at line 92. Ifuuidis not provided, the API call will likely fail. Ensure that a validuuidis always passed as a prop.src/components/CreateCertificate/CreateCertificate.tsx (3)
43-57: The new state variable
openMetadataModalhas been introduced to handle the display of a metadata modal. Ensure that this state variable is being updated correctly throughout the component and that it is being reset when necessary.119-125: Ensure that the recursive call to
fetchRunDetails()does not lead to an infinite loop in case of repeated failures. Consider adding a retry limit or delay increment.157-218: The conditional rendering based on
subscribedFeaturesis a good practice for feature flagging. However, ensure that theindexOfcheck is robust against potential changes in thesubscribedFeaturesarray structure or content.
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Outdated
Show resolved
Hide resolved
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Outdated
Show resolved
Hide resolved
src/pages/certification/components/certificationMetadata/CertificationMetadata.tsx
Show resolved
Hide resolved
|
After I bought the subscription, I had to refresh several times to see the Auditing menu |
|
Validation error in the case of valid value |
|
I wasn't able to submit it even if the form didn't show any validation error or unfilled mandatory field Screen.Recording.2023-09-08.at.14.17.01.mov |
|
Subject inpute accepts in the field invalid format 
Please consult the 
|
Already resolved with a temporary hack (since in revamp the handling of subscriptions will be moved to before entering the pages) in PR#10 PLT-7439 - Fix for Blank page in the tool at times
:) See it isn't a valid value. A valid http(s):// URL must have .json or .pdf.
Thought is was resolved, checking it, will resolve. Issue - Script Hash not showing validation errors
Per PLT-4386 - Subject [1-64 chars], and also discussion with @RSoulatIOHK (missed to pass the info to you 🙏 ) |
an URL could be a PDF without being specified as so. Please follow the Regex pattern specified by the Swagger for that field or let's agree on changing in the BE, anyway both FE and BE should follow the same validation pattern |
Level 0 certificationI don't get L0 audit when submitting and auditor report for L0 audit Instead I should get: Note: Level 1 certificationI get "Undefined" in on/off chain metadata Level 2 certificationEverything looks OK. Just the need for BE to change |
|
In DAPP Script (which should be DApp script I think from IOG standard), there is no display when a regex fails. But you can't submit the report so the check is actually done. I found that repository URL needs to be a valid URL and Script Hash needs to be a valid Hash. Contract address seems to have no check. Don't force the "@" for twitter handle. I know I'm the one that added it a long time ago. Don't blame me plz :'( To Sum up
|
|
Can you fix the validation in BE. The regex used in FE is
/^(https?://)?(www.)?[-a-zA-Z0-9@:%.+~#=]{1,255}.[a-z]{2,6}(\b([-a-zA-Z0-9@:%+.~#()?&//=]*))?$/
Contact Address under DApp Script will be validated across pattern
Able to see errors now with a minor fix, unable to reproduce the scenario when the errors weren't visible. Can you recheck review-cert-metadata.mp4Note -
|
…te + minor schema fix + renamed Reset to Cancel
Resolved this issue - at least by handling the response, and throwing an error to retry (instead of downloading invalid JSONs) PR is ready for re-review |
|
I'll pull it this morning for test. Which BE version should I be using? |
|
On a failed testing, we should not be able to review Certification metadata because we should not issue any. However we should be able to view the full html report, we currently only view the failed unit tests: @amnambiar, @EehMauro Do you want me to move those to Issues so we can revisit that after the revamp? |
|
I don't know if the online deployment is the right BE but I can't get "L1" metadata. I get "Something went wrong message" Also, when you get this message, you can never retry as it says " Forbidden - Certification already started". This should fix. I can also get this as an issue to revisit after revamp |
Due to 204 no content in POST api. Must be because the BE API is not the right one. Neither of the deployments excuse.ro nor aws has the right branch for this. Bogdan needs to confirm the branch that this needs to be verified across.
Is it OK to ensure this in revamp?
Need some clarifications over the workflow, will address in revamp once that is clear
Will handle in revamp, as this will be a tabular view |
There was a problem hiding this comment.
The report upload schema is not in accordance with the latest BE AutditorInput expected schema. @RSoulatIOHK @amnambiar for any changes in BE please comment in order to make the changes.
I will write down what BE expects for the moment and what is different in FE :
certificationLevelis expected to be anumbernow, not astring. I'm looking in thereportUpload.schema.tsx, is the transformation to the number made later?summaryis not required on the BE- same for
disclaimer subjectmatches^[A-Za-z0-9_]{1,64}$( I know it fits the current one, but let's keep the same regex on both sides.discordis different on BE (^.{3,32}#[0-9]{4}$) but I guess that's where the modification should be changed, not in the FE- the
reportUrlis different and has a pdf or txt mandatory suffix, though, a valid pdf URL could not necessarily include thepdf/txtextension. Should we enforce this on BE as well? repositoryin CIP has no validation pattern, in FE has/^(?:https?:\/\/)?(?:www\.)?github\.com\/[\w-]+\/[\w.-]+$/. @RSoulatIOHK what solution should we choose:- we can keep it as it is in both places
- we can remove the validation in FE
- we can add the validation in the BE
UPDATE 1: I verified certificationLevel it's transmitted as number, it's ok
UPDATE 2:
social.website,social.github,social.linkgets the same value from thewebsiteform field- the
social.linkdoes not exist in CIP or BE social.githubon the backend is just the account name with pattern^(?=.{1,39}$)[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$
UPDATE 3:social.discordis not sent into the request
It should be, sorry for missing that
Same, it should be.
Agreed
I don't have a strong opinion but let's take the one from CIP-0096.
Good point, no then. Let's have the most open solution in place and remove the pdf/txt mandatory suffixes and maybe the report could be a json file as well so let's not complicate at the moment.
Yes but we have one that is imposed by our tool only being able to verify github repos at the moment. |
To fix then.
Good catch, it got removed. |
Updated the FE validation to be Other fixes done -
|
…es to auditor profile in revamp
|
Created https://input-output.atlassian.net/browse/PLT-7608 to ensure/handle fixes in revamp |
https://input-output.atlassian.net/browse/PLT-6763
Across BE task - PLT-6367 / PR #74 (merged into
master)BE Revised fixes - feat/l1-certificate PR #90
@coderabbitai:ignore
Summary by CodeRabbit
CertificationFormcomponent for dynamic certification form handling.CertificationMetadatacomponent to submit and manage certification metadata.CertificationMetadataintoCreateCertificatecomponent, allowing users to review and submit certification metadata.ReportUploadcomponent to use the newCertificationFormcomponent, improving code modularity and maintainability.CreateCertificatecomponent with improved state management and error handling.@coderabbitai: ignore