-
Notifications
You must be signed in to change notification settings - Fork 2
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: ✨ Codefair v3.2.0 #114
Conversation
* merge: 🔀 merge main to staging * refactor: ♻️ 🐛 Codefair 3.1.0 (#89) * fix: 🐛 patch removing pull_request.closed bug on metadata pr merge * fix: 🐛 don't update firstPublished in the codemeta file + better logs * fix: 🐛 patch error on reading db entry when it doesn't exist * chore: 🔊 better log on successful Zenodo publish * style: 🎨 margins between text and icon * feat: ✨ links to the list of tools used for codefair in /codefair * refactor: ✨ ♻️ abstract the license validation fn * doc: 🔖 update changelog * feat: ✨ ♻️ abstraction to zenodo workflow * wip: 🚧 abstracting zenodo workflow * feat: ✨ add help link in the Zenodo release page * refactor: ♻️ ✨ abstract out github release functions * refactor: 🔊 ♻️ improve error tracing in metadata file * chore: 🔨 remove todo * refactor: ✨ ♻️ abstracted Zenodo workflow (bot) * wip: 🚧 save custom license content when no assertion is provided * ✨ feat: add support for custom licenses (#90) * ✨ feat: add support for custom licenses * 🐛 fix: reset citation license field * 🐛 fix: update zenodo workflow for custom licenses * fix: 🐛 license is valid with custom license * feat: ✨ custom license template * ✨ feat: update codemeta, license and cwl ui paths (#91) * ✨ feat: update meta paths for consistency * 🚚 fix: updatel icense request path * ✨ feat: add support for custom license title (#92) * refactor: ♻️ ✨ new patches for license, cwl, metadata UI's * refactor: ✨ ♻️ apply migrations from UI to bot * feat: ✨ request custom license title from user * refactor: ♻️ hide message box when custom license title is blank * refactor: ♻️ issue dashboard requests custom title * feat: ✨ Saving custom title (#93) * feat: ✨ save custom title option * refactor: :recyle: reuse saveLicenseDraft for storing custom license title * refactor: ♻️ join draft and save title buttons together * refactor: ♻️ use naive-ui's properties for styling * chore: 🔨 remove typo * chore: 🔨 remove unused variables * feat: ✨ toasts for custom title saved button * refactor: ♻️ function rename * feat: ✨ create Zenodo metadata fn handles custom licenses * feat: ✨ custom license reference for archival workflow * refactor: ♻️ stop creating identifiers for new db entries * refactor: ♻️ 🐛 revert allowing custom licenses for zenodo * refactor: ♻️ Custom License Support (#94) * refactor: ♻️ ✨ custom title saved updates dashboard, disable create release btn when custom license * refactor: ♻️ block progress of fair release if license is custom * feat: ✨ listener for when custom license title is saved through UI * feat: ✨ prevent fair release if not fair compliant * refactor: ♻️ improve license validation and update template text for custom licenses * refactor: ♻️ re-render issue from ui side * refactor: ♻️ update to message explaining custom licenses * db calls running in parallel * fix: 🐛 prevent Zendo section from being seen when the license is custom * fix: 🐛 trim license content before comparison --------- Co-authored-by: Sanjay Soundarajan <sanjay.16@live.com> * refactor: ♻️ safety checks for protected middleware * refacotr: ♻️ update protectRoute to redirect to login when no user or session * feat: :fix: redirect to login if not signed in * refactor: ♻️ remove import * chore: 🔊 🔨 remove logs * docs: 📝 update changelog * fix: 🐛 check license content all times * fix: 🐛 update on zenodo ui page * feat: ✨ delete branch after pr has been closed/merged * chore: 🔨 spacing in changelog * wip: 🚧 individual validation requests for license and metadata * feat: ✨ license validation endpoint * wip: 🚧 creating updated metadata validation * wip: 🚧 patches for metadata revalidation workflow * wip: 🚧 validation for codemeta * doc: 🔖 jsdoc comments on fns * fix: 🐛 template renderer will stop using old links (#99) * fix: 🐛 rerender template will stop using old links * doc: 📝 update CHANGELOG * doc: 📝 update metadata * fix: 🐛 preserve authors and contributors from codemeta * feat: ✨ re-validated codemeta from repo dashboard * fix: 🐛 await metadata validation * feat: ✨ codemeta validation extended with microservice validator * refactor: ♻️ update timestamps to be unix format * feat: ✨ clean privatekey env before use * refactor: ♻️ update cwl validation endpoint * wip: 🚧 send validation message to db * feat: ✨ create migrations for metadata validation messages * wip: 🚧 creating json schema for codemeta.json validations * 👷 ci: setup deployment environments (#101) * 👷 ci: setup for staging env (#103) Co-authored-by: Sanjay Soundarajan <sanjay.16@live.com> Co-authored-by: slugb0t <wheresdorian@gmail.com> fix: 🐛 template renderer will stop using old links (#99) * fix: 🐛 adjust env variables used after env changes * 👷 ci: cleanup dockerfile * feat: ✨ codemeta schema first draft * 👷 ci: cleanup dockerfile * 👷 ci: cleanup dockerfile * 👷 ci: cleanup dockerfile * 👷 ci: setup for staging env * 👷 ci: setup for staging env * 👷 ci: setup for staging env * 👷 ci: setup for staging env * 👷 ci: cleanup dockerfile * 👷 ci: fix staging deploys (#104) * wip: 🚧 abstracting cwl workflow * refactor: ♻️ update codemeta schema to validate against the raw codemeta.json files * refactor: ♻️ update codemeta schema for additional fields * feat: ✨ validating raw content of codemeta.json file for 3.0 * refactor: ♻️ create required fields for codemeta schema * wip: 🚧 testing validation responses for db * handle codemeta.json 3.0 and 2.0 versioning * fix: 🐛 correct branch watch for staging deployment action * doc: 🔖 update changelog for 3.2.0 release * feat: ✨ align bot prisma with ui * fix: 🐛 bug patch for cwl workflow * feat: ✨ metadata workflow update * refactor: ♻️ preserving authors and contributors * wip: 🚧 update the metadata workflow after discussions * fix: 🐛 don't create cwlobject twice * refactor: ♻️ send 200 status when invalid codemeta * refactor: ♻️ update codemeta validation schema * refactor: ♻️ do not revalidate metadata files for push events unless metadata files are updated themselves * wip: 🚧 seperate rerun validation with regather information * wip: 🚧 remove command from issue dashboard if error occurs (allows retry) * refactor: ♻️ update the issue body after successful validation (remove the command from the issue body) * refactor: ♻️ add additional key to codemeta schema * refactor: ♻️ remove the regather options from the UI as validation is still needed when regathering * fix: 🐛 pr button updated with new links * fix: 🐛 patch variable declaration not in scope * refactor: ♻️ update on the dropdown icons * refactor: ♻️ update the keys of the cwl object * refactor: ♻️ update the getcwlfiles function --------- Co-authored-by: Sanjay Soundarajan <sanjay.16@live.com>
* fix: 🐛 correct cwlObject variables in push event * fix: 🐛 better error message in metadata workflow + collect missing fields from codemeta.json * fix: 🐛 apply await to async function call * refactor: ♻️ removed unused imports * refactor: ♻️ add try catch in then statement of promise * fix: 🐛 missing fields in codemeta gathering + add error handling for convertCitationForDB fn * refactor: ♻️ add try catch to renderer to preserve errors * fix: 🐛 preserve metadata validation results in db * wip: 🚧 validation results being displayed through the repo dashboard * wip: 🚧 displaying metadata validation results in seperate pages * feat: ✨ validation page for codemeta * fix: 🐛 patch fundingCode not being preserved from codemeta * wip: 🚧 final design for validation results * feat: ✨ add view validation results conditionally * test: ⚗️ ensuring validation progresses accordingly * feat: ✨ popover messages for metadata badges * feat: ✨ if metadata file fails to parse then update validation message * fix: 🐛 update schema to handle 2.0 and 3.0 authors * style: 🎨 formatting * fix: 🐛 add releaseNotes key to schema for 3.0 versions * refactor: ✏️ change wording * fix: 🐛 validate metadata file that was updated only on push events * chore: 📝 remove test logs
Thank you for submitting this pull request! We appreciate your contribution to the project. Before we can merge it, we need to review the changes you've made to ensure they align with our code standards and meet the requirements of the project. We'll get back to you as soon as we can with feedback. Thanks again! |
Reviewer's Guide by SourceryThis pull request implements Codefair v3.2.0, introducing metadata validation capabilities and improving the handling of metadata files. The changes include a new validator service for codemeta.json and CITATION.cff files, enhanced error handling, and UI improvements for displaying validation results. Class diagram for metadata conversion functionsclassDiagram
class MetadataConverter {
+convertCodemetaForDB(codemetaContent, repository)
+convertCitationForDB(citationContent, repository)
+validateMetadata(metadataInfo, fileType, repository)
+applyCodemetaMetadata(codemeta, metadata, repository)
+applyCitationMetadata(citation, metadata, repository)
}
class DatabaseInstance {
+codeMetadata
+licenseRequest
+cwlValidation
}
MetadataConverter --> DatabaseInstance : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Thanks for making updates to your pull request. Our team will take a look and provide feedback as soon as possible. Please wait for any GitHub Actions to complete before editing your pull request. If you have any additional questions or concerns, feel free to let us know. Thank you for your contributions! |
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.
Hey @slugb0t - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Remove debug logging statement (link)
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🔴 Security: 1 blocking issue, 2 other issues
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
if (file.type === "file" && file.name.endsWith(".cwl")) { | ||
cwlObject.files.push(file); | ||
} |
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.
🚨 suggestion (security): CWL file path validation needs to handle special characters
Add validation/sanitization for file paths that may contain special characters to prevent potential issues with file handling
if (file.type === "file" && file.name.endsWith(".cwl")) { | |
cwlObject.files.push(file); | |
} | |
// Helper function to validate file paths | |
const isValidFilePath = (filename) => { | |
// Check for common problematic characters and patterns | |
const invalidChars = /[<>:"|?*\x00-\x1F]/g; | |
const hasInvalidChars = invalidChars.test(filename); | |
const hasDoubleDots = filename.includes('..'); | |
const hasLeadingSlash = filename.startsWith('/'); | |
return !hasInvalidChars && !hasDoubleDots && !hasLeadingSlash; | |
}; | |
if (file.type === "file" && file.name.endsWith(".cwl")) { | |
if (isValidFilePath(file.name)) { | |
cwlObject.files.push(file); | |
} else { | |
console.warn(`Skipping file with invalid path: ${file.name}`); | |
} | |
} |
validator/app.py
Outdated
|
||
# Only allow CORS origin for localhost:3000 | ||
# and any subdomain of azurestaticapps.net/ | ||
CORS( |
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.
🚨 suggestion (security): Add security headers to Flask application
Consider adding security headers like X-Content-Type-Options, X-Frame-Options, and Content-Security-Policy. These help protect against common web vulnerabilities.
Suggested implementation:
# Only allow CORS origin for localhost:3000
# and any subdomain of azurestaticapps.net/
@app.after_request
def add_security_headers(response):
"""Add security headers to each response."""
# Prevent MIME type sniffing
response.headers['X-Content-Type-Options'] = 'nosniff'
# Prevent clickjacking
response.headers['X-Frame-Options'] = 'SAMEORIGIN'
# Enable XSS filter
response.headers['X-XSS-Protection'] = '1; mode=block'
# Strict CSP
response.headers['Content-Security-Policy'] = "default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline';"
# HSTS (uncomment if you're using HTTPS)
# response.headers['Strict-Transport-Security'] = 'max-age=31536000; includeSubDomains'
return response
CORS(
Note: The Content-Security-Policy header provided is a basic secure default. You may need to adjust it based on your application's specific needs, such as:
- Adding additional domains to script-src if you use external JavaScript
- Configuring img-src if you load images from external sources
- Adding connect-src directives for your API endpoints
- Uncommenting HSTS if you're using HTTPS (recommended for production)
|
||
## Fixed | ||
|
||
- Fixed issues related to filtering what is consider a cwl file in the repository. |
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.
issue (typo): Grammar issue: 'consider' should be 'considered'
- Fixed issues related to filtering what is consider a cwl file in the repository. | |
- Fixed issues related to filtering what is considered a cwl file in the repository. |
validator/README.md
Outdated
|
||
### Setup | ||
|
||
If you would like to update the api, please follow the instructions below. |
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.
issue (typo): 'api' should be capitalized as 'API'
If you would like to update the api, please follow the instructions below. | |
If you would like to update the API, please follow the instructions below. |
roles: [], // Roles will be added later | ||
uri: contributor?.id || "", | ||
}); | ||
export async function convertCodemetaForDB(codemetaContent, repository) { |
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.
issue (complexity): Consider extracting duplicated person handling logic into a reusable function to process both authors and contributors.
The person handling logic in convertCodemetaForDB
is overly complex with duplicated code for authors and contributors. Consider extracting this into a reusable function:
function processPersons(persons, type = 'author') {
const processed = [];
// Process base person records
persons?.forEach(person => {
if (person.type === "Person" || person.type === "Organization") {
processed.push({
affiliation: person?.affiliation?.name || "",
email: person?.email || "",
familyName: person?.familyName || "",
givenName: person?.givenName || "",
roles: [],
uri: person?.id || ""
});
}
});
// Process roles
persons?.forEach(person => {
if (person.type === "Role") {
processed.forEach(proc => {
const personId = type === 'author' ? person?.["schema:author"] :
(person?.contributor || person?.["schema:contributor"]);
if (proc.uri === personId) {
proc.roles.push({
role: person.roleName || "",
startDate: person.startDate ? convertDateToUnix(person.startDate) : null,
endDate: person.endDate ? convertDateToUnix(person.endDate) : null
});
}
});
}
});
return processed;
}
This would simplify the main function:
const sortedAuthors = processPersons(codemetaContent?.author, 'author');
const sortedContributors = processPersons(codemetaContent?.contributor, 'contributor');
This refactoring preserves all functionality while making the code more maintainable and easier to test.
} | ||
}); | ||
|
||
const license = licenseRequest.data.license ? true : false; |
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.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary
)
const license = licenseRequest.data.license ? true : false; | |
const license = !!licenseRequest.data.license; |
Explanation
It is possible to simplify certain ternary statements into either use of an||
or !
.This makes the code easier to read, since there is no conditional logic.
bot/metadata/index.js
Outdated
if (metadata.authors) { | ||
if (convertedCodemeta.authors.length > 0) { | ||
const updatedAuthors = convertedCodemeta.authors.map((author) => { | ||
// Find a matching author in metadata | ||
const foundAuthor = metadata.authors.find( | ||
(existingAuthor) => | ||
existingAuthor?.familyName === author?.familyName && | ||
existingAuthor?.givenName === author?.givenName | ||
); | ||
|
||
if (foundAuthor) { | ||
// Merge roles, avoiding duplicates based on `role` and `startDate` | ||
if (!foundAuthor?.roles) { | ||
foundAuthor.roles = []; | ||
} | ||
const mergedRoles = [ | ||
...foundAuthor.roles, | ||
...author.roles.filter( | ||
(newRole) => | ||
!foundAuthor.roles.some( | ||
(existingRole) => | ||
existingRole.role === newRole.role && | ||
existingRole.startDate === newRole.startDate | ||
) | ||
) | ||
]; | ||
|
||
// Merge and prioritize data from `author` | ||
return { | ||
...foundAuthor, | ||
...author, | ||
affiliation: author.affiliation || foundAuthor.affiliation || "", | ||
email: author.email || foundAuthor.email || "", | ||
uri: author.uri || foundAuthor.uri || "", | ||
roles: mergedRoles | ||
}; | ||
} | ||
|
||
// If no match, return the current author from convertedCodemeta | ||
return author; | ||
}); | ||
|
||
// Merge updated authors with any authors in metadata not present in convertedCodemeta | ||
const nonUpdatedAuthors = metadata.authors.filter( | ||
(existingAuthor) => | ||
!convertedCodemeta.authors.some( | ||
(author) => | ||
author.familyName === existingAuthor.familyName && | ||
author.givenName === existingAuthor.givenName | ||
) | ||
); | ||
|
||
metadata.authors = [...nonUpdatedAuthors, ...updatedAuthors]; | ||
} | ||
} |
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.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs
)
if (metadata.authors) { | |
if (convertedCodemeta.authors.length > 0) { | |
const updatedAuthors = convertedCodemeta.authors.map((author) => { | |
// Find a matching author in metadata | |
const foundAuthor = metadata.authors.find( | |
(existingAuthor) => | |
existingAuthor?.familyName === author?.familyName && | |
existingAuthor?.givenName === author?.givenName | |
); | |
if (foundAuthor) { | |
// Merge roles, avoiding duplicates based on `role` and `startDate` | |
if (!foundAuthor?.roles) { | |
foundAuthor.roles = []; | |
} | |
const mergedRoles = [ | |
...foundAuthor.roles, | |
...author.roles.filter( | |
(newRole) => | |
!foundAuthor.roles.some( | |
(existingRole) => | |
existingRole.role === newRole.role && | |
existingRole.startDate === newRole.startDate | |
) | |
) | |
]; | |
// Merge and prioritize data from `author` | |
return { | |
...foundAuthor, | |
...author, | |
affiliation: author.affiliation || foundAuthor.affiliation || "", | |
email: author.email || foundAuthor.email || "", | |
uri: author.uri || foundAuthor.uri || "", | |
roles: mergedRoles | |
}; | |
} | |
// If no match, return the current author from convertedCodemeta | |
return author; | |
}); | |
// Merge updated authors with any authors in metadata not present in convertedCodemeta | |
const nonUpdatedAuthors = metadata.authors.filter( | |
(existingAuthor) => | |
!convertedCodemeta.authors.some( | |
(author) => | |
author.familyName === existingAuthor.familyName && | |
author.givenName === existingAuthor.givenName | |
) | |
); | |
metadata.authors = [...nonUpdatedAuthors, ...updatedAuthors]; | |
} | |
} | |
if (metadata.authors && convertedCodemeta.authors.length > 0) { | |
const updatedAuthors = convertedCodemeta.authors.map((author) => { | |
// Find a matching author in metadata | |
const foundAuthor = metadata.authors.find( | |
(existingAuthor) => | |
existingAuthor?.familyName === author?.familyName && | |
existingAuthor?.givenName === author?.givenName | |
); | |
if (foundAuthor) { | |
// Merge roles, avoiding duplicates based on `role` and `startDate` | |
if (!foundAuthor?.roles) { | |
foundAuthor.roles = []; | |
} | |
const mergedRoles = [ | |
...foundAuthor.roles, | |
...author.roles.filter( | |
(newRole) => | |
!foundAuthor.roles.some( | |
(existingRole) => | |
existingRole.role === newRole.role && | |
existingRole.startDate === newRole.startDate | |
) | |
) | |
]; | |
// Merge and prioritize data from `author` | |
return { | |
...foundAuthor, | |
...author, | |
affiliation: author.affiliation || foundAuthor.affiliation || "", | |
email: author.email || foundAuthor.email || "", | |
uri: author.uri || foundAuthor.uri || "", | |
roles: mergedRoles | |
}; | |
} | |
// If no match, return the current author from convertedCodemeta | |
return author; | |
}); | |
// Merge updated authors with any authors in metadata not present in convertedCodemeta | |
const nonUpdatedAuthors = metadata.authors.filter( | |
(existingAuthor) => | |
!convertedCodemeta.authors.some( | |
(author) => | |
author.familyName === existingAuthor.familyName && | |
author.givenName === existingAuthor.givenName | |
) | |
); | |
metadata.authors = [...nonUpdatedAuthors, ...updatedAuthors]; | |
} | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if
conditions can be combined usingand
is an easy win.
try: | ||
file_content = json.loads(file_content) | ||
except Exception as e: | ||
return { |
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.
issue (code-quality): We've found these issues:
- Use f-string instead of string concatenation [×3] (
use-fstring-for-concatenation
) - Simplify conditional into switch-like form [×3] (
switch
)
Thanks for closing this pull request! If you have any further questions, please feel free to open a new issue. We are always happy to help! |
Summary by Sourcery
Introduce metadata and license validation features, enhance the UI for displaying validation results, and set up deployment for the validator service using Kamal and Docker.
New Features:
Bug Fixes:
Enhancements:
CI:
Deployment:
Documentation:
Tests: