PLT-6763 FE: Auditors sign certificates

amnambiar commented 1 year ago

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

New Feature: Introduced CertificationForm component for dynamic certification form handling.
New Feature: Added CertificationMetadata component to submit and manage certification metadata.
New Feature: Integrated CertificationMetadata into CreateCertificate component, allowing users to review and submit certification metadata.
Refactor: Updated ReportUpload component to use the new CertificationForm component, improving code modularity and maintainability.
Refactor: Enhanced CreateCertificate component with improved state management and error handling.
Style: Adjusted CSS styles across various components for better UI consistency.

@coderabbitai: ignore

coderabbitai[bot] commented 1 year ago

Walkthrough

This pull request introduces a new certification form component with dynamic field handling, and integrates it into the existing CreateCertificate and ReportUpload components. It also adds a metadata modal to the CreateCertificate component and refactors various parts of the code for better readability and maintainability.

Changes

File(s)	Summary
`src/components/CertificationForm/...` `src/pages/certification/components/certificationMetadata/...`	Introduced a new `CertificationForm` component and a `CertificationMetadata` component for handling certification data.
`src/components/CreateCertificate/...` `src/pages/auditing/reportUpload/...`	Refactored the `CreateCertificate` and `ReportUpload` components to use the new `CertificationForm`. Added a metadata modal to the `CreateCertificate` component.
`src/components/DAPPScript/...`	Updated the `DAPPScript` component to render dynamic inputs based on a configuration prop.
`src/components/Modal/Modal.tsx`	Enhanced the `Modal` component with additional props for more customization.
Various `.scss` files	Made several CSS changes to accommodate the new components and enhance the user interface.

"In the land of code, where logic is king, 🤴👑
A rabbit hopped in, with new features to bring. 🐇💼
With forms that grow, and modals that show, 📝🎭
The user's journey, now has a better flow!" 🚀🌈

Tips

### Chat with CodeRabbit Bot (`@coderabbitai`) - Mention `@coderabbitai` in any *review comment* for bot assistance. - Note: Review comments are made on code diffs or files, not on the PR overview. ### Pause Incremental Reviews - Insert `@coderabbitai: ignore` in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

bogdan-manole commented 1 year ago

After I bought the subscription, I had to refresh several times to see the Auditing menu

bogdan-manole commented 1 year ago

Validation error in the case of valid value

bogdan-manole commented 1 year ago

I wasn't able to submit it even if the form didn't show any validation error or unfilled mandatory field

https://github.com/input-output-hk/dapps-certification-web/assets/3176255/f238c1a8-1a50-4086-9383-f071739a8855

bogdan-manole commented 1 year ago

Subject inpute accepts in the field invalid format

Please consult the Swagger api data model specification

amnambiar commented 1 year ago

@bogdan-manole

After I bought the subscription, I had to refresh several times to see the Auditing menu

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

Validation error in the case of valid value

:) See it isn't a valid value. A valid http(s):// URL must have .json or .pdf.

I wasn't able to submit it even if the form didn't show any validation error or unfilled mandatory field

Thought is was resolved, checking it, will resolve. Issue - Script Hash not showing validation errors

Subject inpute accepts in the field invalid format

Per PLT-4386 - Subject [1-64 chars], and also discussion with @RSoulatIOHK (missed to pass the info to you :pray: )

bogdan-manole commented 1 year ago

:) See it isn't a valid value. A valid http(s):// URL must have .json or .pdf.

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

RSoulatIOHK commented 1 year ago

Level 0 certification

I don't get L0 audit when submitting and auditor report for L0 audit {"1304":{"metadata":[["ipfs://bafkreiaerryd4ou6edxrygimjxzgtv4wyjmlhcqqtg47cosz5oygzdnf","le"]],"rootHash":"64346f7e71cdc0b790c3992114f3ab3b15dddb7e4b5ffc10269a06fbb5484828","schemaVersion":1,"subject":"test","type":{"action":"CERTIFY","certificateIssuer":"test","certificationLevel":"l2"}}}

Instead I should get: {"1304":{"metadata":[["ipfs://bafkreiaerryd4ou6edxrygimjxzgtv4wyjmlhcqqtg47cosz5oygzdnf","le"]],"rootHash":"64346f7e71cdc0b790c3992114f3ab3b15dddb7e4b5ffc10269a06fbb5484828","schemaVersion":1,"subject":"test","type":{"action":"**AUDIT**","certificateIssuer":"test","certificationLevel":**0**}}}

Note: certificationLevel should be an int with just the certification level and not a str.

Level 1 certification

I get "Undefined" in on/off chain metadata

Level 2 certification

Everything looks OK. Just the need for BE to change certificationLevel which should be an int with just the certification level and not a str.

RSoulatIOHK commented 1 year ago

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

[x] Change DAPP to DApp
[ ] Display when a regex fails for DApp Scripts (Repository URL and Script hash look missing)
[x] Check that Contract address is a valid address
[x] Remove the needs for an "@" in the twitter handle.

amnambiar commented 1 year ago

@bogdan-manole

https://github.com/input-output-hk/dapps-certification-web/pull/8#issuecomment-1713760452

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@:%_\+.~#()?&\/\/=]*))?\.(?:jpg|jpeg|png|gif|bmp|svg|webp|tiff|tif)$/

Website URL regex pattern

/^(https?:\/\/)?(www.)?[-a-zA-Z0-9@:%.+~#=]{1,255}.[a-z]{2,6}(\b([-a-zA-Z0-9@:%+.~#()?&\/\/=]*))?$/

@RSoulatIOHK ,

Check that Contract address is a valid address

Contact Address under DApp Script will be validated across pattern ^(addr_test1|addr1)[a-zA-Z0-9]{53,}$; and will be non-mandatory

Display when a regex fails for DApp Scripts (Repository URL and Script hash look missing)

Able to see errors now with a minor fix, unable to reproduce the scenario when the errors weren't visible. Can you recheck

https://github.com/input-output-hk/dapps-certification-web/assets/11058223/ee106ddd-2b8f-46f3-989a-c399163e516b

Note -

Received undefined in off-chain and on-chain downloaded JSONs - because the APIs returned 204 No Content and FE didn't handle it properly. I'm on it, will verify once the API service is up and running, and comment
BE is pointed towards https://dapps-certification-web.scdev.aws.iohkdev.io/ now, and not excuse.ro ( it has latest profile/subscription API updates for the moment)

amnambiar commented 1 year ago

@RSoulatIOHK ,

Received undefined in off-chain and on-chain downloaded JSONs - because the APIs returned 204 No Content and FE didn't handle it properly. I'm on it, will verify once the API service is up and running, and comment

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

RSoulatIOHK commented 1 year ago

I'll pull it this morning for test. Which BE version should I be using?

RSoulatIOHK commented 1 year ago

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: vs

@amnambiar, @EehMauro Do you want me to move those to Issues so we can revisit that after the revamp?

RSoulatIOHK commented 1 year ago

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

amnambiar commented 1 year ago

@RSoulatIOHK ,

"Something went wrong message"

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.

On a failed testing, we should not be able to review Certification metadata because we should not issue any.

Is it OK to ensure this in revamp?

After something went wrong message, you can never retry as it says " Forbidden - Certification already started". This should fix.

Need some clarifications over the workflow, will address in revamp once that is clear

However we should be able to view the full html report, we currently only view the failed unit tests:

Will handle in revamp, as this will be a tabular view

RSoulatIOHK commented 1 year ago

summary is not required on the BE

It should be, sorry for missing that

same for disclaimer

Same, it should be.

subject matches ^[A-Za-z0-9_]{1,64}$ ( I know it fits the current one, but let's keep the same regex on both sides.

Agreed

discord is different on BE (^.{3,32}#[0-9]{4}$) but I guess that's where the modification should be changed, not in the FE

I don't have a strong opinion but let's take the one from CIP-0096.

the reportUrl is different and has a pdf or txt mandatory suffix, though, a valid pdf URL could not necessarily include the pdf/txt extension. Should we enforce this on BE as well?

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.

repository in CIP has no validation pattern, in FE has /^(?:https?:\/\/)?(?:www.)?github.com\/[\w-]+\/[\w.-]+$/.

Yes but we have one that is imposed by our tool only being able to verify github repos at the moment.

RSoulatIOHK commented 1 year ago

social.website, social.github, social.link gets the same value from the website form field

To fix then.

the social.link does not exist in CIP or BE

Good catch, it got removed.

amnambiar commented 1 year ago

subject matches ^[A-Za-z0-9_]{1,64}$ ( I know it fits the current one, but let's keep the same regex on both sides.

Updated the FE validation to be .required(...).max(64, ...).matches(/^[A-Za-z0-9_]+$/, .... just to ensure the form errors are thrown properly

Other fixes done -

remove mandatory json/pdf extension checks at the reportUrl
update social.github pattern to ^(?=.{1,39}$)[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$ rather than url
social has properties - twitter, github, contact, website, discord ; fixed value mapping in these fields
ensure optional fields not sent as empty-strings in payload; they are either null or not existing
Github - field was missing in Auditor Report Upload, added it there.

Cc: @bogdan-manole @RSoulatIOHK

amnambiar commented 1 year ago

Created https://input-output.atlassian.net/browse/PLT-7608 to ensure/handle fixes in revamp

input-output-hk / dapps-certification-web