-
Notifications
You must be signed in to change notification settings - Fork 9
feat: [LC-1309][LC-1310] - Add Evidence Support #824
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 2a3cf53 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for learncarddocs canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
The PR implements Evidence support for Open Badges v3 with proper JSON-LD context updates, but there are critical issues with validator consistency and evidence field handling that could break existing functionality.
3 issues detected:
🐞 Bug - Evidence field was removed from achievement credential validator despite the PR adding evidence support
Details: The evidence field has been removed from UnsignedAchievementCredentialValidator while the PR is specifically adding evidence support. This contradicts the core functionality being implemented and will prevent proper validation of evidence in achievement credentials.
File:packages/learn-card-types/src/obv3.ts
🐞 Bug - Potential runtime error when filtering undefined e.type array
Details: The evidence type manipulation logic assumes e.type exists as an array but uses optional chaining, suggesting it might be undefined. The filter operation on a potentially undefined value could cause runtime errors during credential generation.
File:packages/plugins/vc-templates/src/templates.ts (156-162)
🧹 Maintainability - Inconsistent union syntax compared to other optional array fields
Details: The evidence field uses z.union() while other similar optional array fields in the same validator use the .or() method pattern. This inconsistency makes the codebase harder to maintain and understand.
File:packages/learn-card-types/src/vc.ts (163-163)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀
type: e.type?.includes('EvidenceFile') | ||
? e.type | ||
: [ | ||
'Evidence', | ||
'EvidenceFile', | ||
...(e.type?.filter(t => t !== 'Evidence') || []), | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐞 Bug - Unsafe Type Manipulation: Add proper null/undefined checks before the filter operation and ensure e.type is always initialized as an array.
type: e.type?.includes('EvidenceFile') | |
? e.type | |
: [ | |
'Evidence', | |
'EvidenceFile', | |
...(e.type?.filter(t => t !== 'Evidence') || []), | |
], | |
type: Array.isArray(e.type) | |
? e.type.includes('EvidenceFile') | |
? e.type | |
: [ | |
'Evidence', | |
'EvidenceFile', | |
...e.type.filter(t => t !== 'Evidence'), | |
] | |
: ['Evidence', 'EvidenceFile'], |
status: CredentialStatusValidator.or(CredentialStatusValidator.array()).optional(), | ||
termsOfUse: TermsOfUseValidator.or(TermsOfUseValidator.array()).optional(), | ||
evidence: VC2EvidenceValidator.or(VC2EvidenceValidator.array()).optional(), | ||
evidence: z.union([VC2EvidenceValidator, z.array(VC2EvidenceValidator)]).optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Maintainability - Inconsistent Union Syntax: Use consistent syntax by changing to VC2EvidenceValidator.or(VC2EvidenceValidator.array()).optional() to match the pattern used for other similar fields.
evidence: z.union([VC2EvidenceValidator, z.array(VC2EvidenceValidator)]).optional(), | |
evidence: VC2EvidenceValidator.or(VC2EvidenceValidator.array()).optional(), |
🥷 Code experts: TaylorBeeston TaylorBeeston, gerardopar have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
The PR successfully implements evidence support for Open Badges v3 compliance by updating validators, templates, and type definitions. The evidence field has been properly moved from achievement-specific to VC-level validation, which is architecturally correct.
2 issues detected:
🐞 Bug - Optional chaining suggests e.type might be undefined, but subsequent operations assume it's an array
Details: The evidence type manipulation logic uses optional chaining for e.type?.includes() and e.type?.filter() but doesn't handle the case where e.type is undefined. This could cause runtime errors when processing evidence arrays during credential generation.
File:packages/plugins/vc-templates/src/templates.ts (156-161)
🧹 Maintainability - Mixing z.union() and .or() patterns for similar field types creates inconsistent code style
Details: The evidence field uses z.union() while other similar optional array fields in the same validator use the .or() method pattern. This inconsistency makes the codebase harder to understand and maintain.
File:packages/learn-card-types/src/vc.ts (163-163)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀
type: e.type?.includes('EvidenceFile') | ||
? e.type | ||
: [ | ||
'Evidence', | ||
'EvidenceFile', | ||
...(e.type?.filter(t => t !== 'Evidence') || []), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐞 Bug - Potential Runtime Error: Add proper null/undefined checks or provide default values. For example: (e.type || []).includes('EvidenceFile')
and ensure e.type is always an array before filtering.
type: e.type?.includes('EvidenceFile') | |
? e.type | |
: [ | |
'Evidence', | |
'EvidenceFile', | |
...(e.type?.filter(t => t !== 'Evidence') || []), | |
type: (e.type || []).includes('EvidenceFile') | |
? e.type | |
: [ | |
'Evidence', | |
'EvidenceFile', | |
...(e.type || []).filter(t => t !== 'Evidence'), |
status: CredentialStatusValidator.or(CredentialStatusValidator.array()).optional(), | ||
termsOfUse: TermsOfUseValidator.or(TermsOfUseValidator.array()).optional(), | ||
evidence: VC2EvidenceValidator.or(VC2EvidenceValidator.array()).optional(), | ||
evidence: z.union([VC2EvidenceValidator, z.array(VC2EvidenceValidator)]).optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Maintainability - Inconsistent Validation Pattern: Change to use the consistent .or() pattern: evidence: VC2EvidenceValidator.or(VC2EvidenceValidator.array()).optional()
evidence: z.union([VC2EvidenceValidator, z.array(VC2EvidenceValidator)]).optional(), | |
evidence: VC2EvidenceValidator.or(VC2EvidenceValidator.array()).optional(), |
Overview
🎟 Relevant Jira Issues
LC-1309
LC-1310
CLIENT SIDE PR
📚 What is the context and goal of this PR?
This PR introduces full Evidence support into our BoostCredential workflow and updates our VC context to version 1.03. The goal is to make LearnCard-issued credentials more interoperable with external verifiable credentials while still supporting our enhanced metadata needs.
Extended JSON-LD Context (v1.03)
Evidence Integration
Implementation Improvements
🥴 TL; RL:
💡 Feature Breakdown (screenshots & videos encouraged!)
🛠 Important tradeoffs made:
🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
📱 🖥 Which devices would you like help testing on?
🧪 Code Coverage
Documentation
📜 Gitbook
📊 Storybook
✅ PR Checklist
🚀 Ready to squash-and-merge?:
✨ PR Description
Purpose: Add evidence support to Open Badge credentials with enhanced type validation and template functionality.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀