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: add support for optional credit class image #56

Merged
merged 7 commits into from
Jul 3, 2023

Conversation

blushi
Copy link
Member

@blushi blushi commented Jun 15, 2023

Description

ref: regen-network/rnd-dev-team#1428

It also removes some unnecessary @typess : 2872e5e
The impacted metadata also got updated on the prod database (their IRIs remaining the same since those changes are only format related changes and don't have any impact on the resulting canonized data)


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
  • reviewed "Files changed" and left comments if necessary

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
  • reviewed documentation is accurate
  • manually tested (if applicable)

@@ -0,0 +1,63 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this generic credit-class.jsonld template because I thought it would be useful for community credit class creation

Copy link
Member Author

@blushi blushi Jun 15, 2023

Choose a reason for hiding this comment

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

@S4mmyb could you let me know whether we should remove certain fields from here or not? (fields that might not be generic to all credit classes)

@@ -1,77 +1,15 @@
@prefix dash: <http://datashapes.org/dash#> .
Copy link
Member Author

@blushi blushi Jun 15, 2023

Choose a reason for hiding this comment

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

made the existing credit-class.ttl legacy and just documented the optional image for credit class in here
I'm wondering whether I should add properties that we're sure are generic to all credit classes too (like name, description... those needs to be confirmed first, see my comment above)

Copy link
Contributor

Choose a reason for hiding this comment

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

why wouldn't we want to add those @blushi ? i'm honestly just unsure either way, so thought it would be helpful to ask.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that we could have one generic credit-class.ttl file that documents all the standardized fields for credit class metadata, then the non standardized fields (relative to a particular credit class) would be in separate .ttl files
Because right now, most of the constraints in C01-verified-carbon-standard-class.ttl, C02-city-forest-credits-class.ttl and C03-toucan-tco2-class.ttl are repeated

Copy link
Member Author

Choose a reason for hiding this comment

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

done

"regen:sectoralScope": [
""
],
"regen:offsetGenerationMethod": [
Copy link
Member Author

Choose a reason for hiding this comment

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

probably we should start using regen:creditGenerationMethod instead to be more generic like discussed here: https://www.notion.so/regennetwork/Offset-Generation-Method-9d2839af4bcb42a197f019a1b131a149?pvs=4

Copy link
Contributor

Choose a reason for hiding this comment

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

is it a good time to do that in this PR? or does it maybe make sense to do it all at once in a separate task? i'm just thinking of the cascade of changes we would have to make if we did do it as a part of this (and assuming we want to deprecate the regen:offsetGenerationMethod in general).

Copy link
Contributor

Choose a reason for hiding this comment

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

but if what you had in mind was to add this field here and we would still keep regen:offsetGenerationMethod as a field, that would be less work

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah my suggestion was still to support regen:offsetGenerationMethod for existing credit classes so we don't have to update their metadata but use regen:creditGenerationMethod moving forward, especially for community credit classes. This way that doesn't imply additional work (except for some minor front-end changes).

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm ok with that then, makes sense to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this naming was coming from Sarah B. but it is a bit confusing for me because on the buyers page we do use "offset generation method" which she did approve and I think we should keep it consistent. If offset generation method is more clear, then maybe we just wouldn't show that field for biodiversity credits, water credits, etc. rather than trying to make it generic?

Makes sense, in this case, should we revert to "offset generation method" on the credit class card? it's currently on dev: https://dev.app.regen.network/project/C02-001

Choose a reason for hiding this comment

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

Yes I guess so

Copy link
Member

Choose a reason for hiding this comment

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

Non-carbon credits aren't considered offsets, so I think we should just exclude "offset generation method" for non-carbon credits. The ERABrazil example is wrong because it shouldn't be included as it's not an offset, so I think we should just put a N/A in those cases like you did @blushi. In terms of the terminology, I think "offset generation method" is fine. But if we're looking for something generic that we could put on all credit class detail cards I think I would go with Project Activity over "offset generation method" as it's something we can stick on carbon and non-carbon credit class cards.

Copy link
Member Author

Choose a reason for hiding this comment

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

re: ERA Brazil, @erikalogie created an issue to make the offset gen method optional for project cards on the buyers because so far they were required (from Sanity): regen-network/rnd-dev-team#1751

Copy link
Member Author

Choose a reason for hiding this comment

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

Also added regen-network/regen-web#1952 for reverting the naming on the app

@blushi blushi requested review from wgwz, clevinson and S4mmyb June 15, 2023 07:59
@blushi blushi changed the title feat: add support for optional class image feat: add support for optional credit class image Jun 15, 2023
@blushi
Copy link
Member Author

blushi commented Jun 15, 2023

@wgwz could you help with the docker build issue: https://github.com/regen-network/regen-registry-standards/actions/runs/5276167983/jobs/9542523640?pr=56? thanks!

@wgwz
Copy link
Contributor

wgwz commented Jun 15, 2023

@blushi no prob, it was just a new release of a debian docker image that we automatically upgraded to causing the issue. i pushed a commit to pin us to the debian release that we were using previously (bullseye).

@blushi blushi mentioned this pull request Jun 22, 2023
6 tasks
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.

4 participants