Skip to content
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: project forms validation #1794

Merged
merged 10 commits into from
Feb 27, 2023
Merged

Conversation

blushi
Copy link
Member

@blushi blushi commented Feb 20, 2023

Description

Closes: regen-network/rnd-dev-team#1501


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed
  • once the PR is closed, set up backport PRs for redwood, hambach and optionally master (see below)

Setting up backport PRs

After merging your PR to dev, set up backports by doing the following:

  1. If your branch does not have merge commits, add the following comment to
    your PR:
  • If PR is a new feature: @Mergifyio backport redwood hambach.
  • If PR is a bug fix: @Mergifyio backport redwood hambach master.
  1. If your branch does have merge commits:

    a. Pull latest dev, hambach and redwood branches

    b. Create new branches for backports and merge dev (replace <PR#> with your PR #)

    git checkout -b hambach-backport-<PR#> hambach
    git merge dev
    git push origin hambach-backport-<PR#>
    git checkout -b redwood-backport-<PR#> redwood
    git merge dev
    git push origin redwood-backport-<PR#>`
    

    c. Open new PRs in regen-web targeting hambach and redwood, respectively.

    d. If PR is a bug fix repeat the same process for master branch.

How to test

Create a new project from https://deploy-preview-1794--regen-registry.netlify.app/ecocredits/projects (using our test addr regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46) and go through the different forms to make sure the validation is working correctly.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

@blushi blushi changed the base branch from main to dev February 20, 2023 13:25
@@ -0,0 +1 @@
export const DESCRIPTION_MAX_LENGTH = 1400;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikalogie I'm not sure what is the actual description max length we want, on the figma it's less than that but in the SHACL definitions, you set it to 1400 @wgwz

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blushi I think that 1400 is probably fine if we have a "read more" link implemented on the front-end, which it looks to me like we don't have anymore (or maybe we only ever had it for the "additional story" section?). Otherwise maybe we should limit it to more like 1,000 characters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we would have the read more though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikalogie looking at the code, we currently have the "read more" only for the land steward story but not for the generic description, I can create an issue to handle that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can't recall a reason that i set it to 1400, i probably was just copying from something that already existed in the standards. just wanted to mention that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blushi blushi force-pushed the marie/feat-1501-project-forms-validation branch from 78c6901 to 0beb075 Compare February 20, 2023 16:12
@blushi blushi requested a review from a team February 21, 2023 07:54
@blushi blushi force-pushed the marie/feat-1501-project-forms-validation branch from 0beb075 to 9026562 Compare February 21, 2023 11:49
@blushi
Copy link
Member Author

blushi commented Feb 21, 2023

@erikalogie also see testing instructions

const metadata = getProjectCreateBaseData(creditClassOnChainId);
setCreditClassId(creditClassOnChainId);
const metadata = getProjectCreateBaseData(creditClassOnChainId);
setCreditClassId(creditClassOnChainId);
Copy link
Contributor

@wgwz wgwz Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just wanted to make a note here that i would like to understand this part better, i'm having trouble visualizing what the actions in UI are that correspond to this code (choosing a credit class and why we update project). i'd like to understand this better. i'm just missing some context. not a blocker on this by any means

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

choosing a credit class corresponds to this page: https://www.figma.com/file/TyhFaXqko9F8OvLEE4Zomg/Issuer-Create%2FEdit-Projects%2C-Credit-Class%2C-Batches?node-id=2%3A4630&t=gYtGMt1zelv99hxH-0, this is the very first form you get when creating a new project
we update the off-chain project to save the chosen credit class

Copy link
Contributor

@wgwz wgwz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! i think it was a good choice to redo this validation with yup (and eventually zod). it's simpler code, easier to maintain.

Copy link
Collaborator

@flagrede flagrede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

web-registry/src/lib/rdf/rdf.ts Outdated Show resolved Hide resolved
@blushi blushi force-pushed the marie/feat-1501-project-forms-validation branch from d2bb3cb to 8b2ed24 Compare February 27, 2023 08:45
@erikalogie
Copy link
Collaborator

@blushi is this what's expected?

image

@blushi
Copy link
Member Author

blushi commented Feb 27, 2023

@blushi is this what's expected?

image

yes for C01 projects, the metadata should have those fields that are missing
if you add "regen:projectActivity": "lorem ipsum" to the JSON then the error regarding this particular field should disappear for instance

@erikalogie
Copy link
Collaborator

@blushi ok great, then this looks like it is working as expected

@blushi blushi merged commit 7335597 into dev Feb 27, 2023
@blushi blushi deleted the marie/feat-1501-project-forms-validation branch February 27, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update project forms validation to account for anchored/unanchored metadata
4 participants