Add certificate settings for automation#17
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR adds support for configurable certificate settings during course creation and update processes. Users can now specify certificate enablement, criteria type (completion/points), and criteria value directly via GitHub Action inputs or CLI arguments, which get included in the API payload sent to qBraid.
Changes:
- Added three new action inputs (
certificate-enabled,certificate-criteria-type,certificate-criteria-value) toaction.yml, propagated throughcreate_course.pyandupdate_course.pyCLI argument parsers and class constructors - Implemented
validate_certificate_settings,validate_certificate_criteria_type,validate_certificate_criteria_value, andbuild_certificate_settingsindeploy_common.py, and updatedget_common_payloadto includecertificateSettingsin the API payload - Updated
README.md,AGENTS.md,examples/workflow.yml, andexamples/example.yamldocumentation to cover the new certificate inputs
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/scripts/common.py |
Swaps production API URL for a local ngrok development URL (critical regression) |
src/scripts/deploy_common.py |
Adds certificate validation/building functions and updates CourseDeployer to accept and forward certificate_settings |
src/scripts/create_course.py |
Adds certificate CLI arguments and passes certificate_settings through to CourseCreator |
src/scripts/update_course.py |
Adds certificate CLI arguments and passes certificate_settings through to CourseUpdater |
action.yml |
Adds three new certificate-related inputs and passes them to both create/update stages |
README.md |
Adds a Certificate Settings section with usage examples |
AGENTS.md |
Documents the three new certificate inputs in the optional inputs table |
examples/workflow.yml |
Adds certificate input examples to the workflow template |
examples/example.yaml |
Adds certificate input examples to the example workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/scripts/deploy_common.py
Outdated
|
|
||
| If enabled is False, returns settings with enabled=False. | ||
| """ | ||
| settings: Dict[str, Any] = {"enabled": enabled} | ||
|
|
||
| if enabled: | ||
| criteria: Dict[str, Any] = {"type": criteria_type} | ||
| if criteria_value is not None: | ||
| if criteria_type == "completion" and criteria_value > 100: | ||
| raise ValidationError( | ||
| "Certificate criteria value cannot exceed 100 for completion type" | ||
| ) | ||
| criteria["value"] = criteria_value | ||
| settings["criteria"] = criteria | ||
|
|
There was a problem hiding this comment.
build_certificate_settings always returns a non-empty dict (e.g., {"enabled": False} when enabled=False). Since a non-empty dict is truthy in Python, the condition if self.certificate_settings in get_common_payload at deploy_common.py:249 will always be True for any call originating from the CLI path (including the default certificate-enabled: 'false' case). This means certificateSettings: {"enabled": false} will be sent in every API payload for courses, even when the user has not explicitly configured certificates. If the API treats an absent certificateSettings field differently from {"enabled": false}, this could produce unintended behavior for users who do not set any certificate inputs.
Consider returning None from build_certificate_settings when enabled is False, or changing the truthiness check in get_common_payload to if self.certificate_settings is not None.
| If enabled is False, returns settings with enabled=False. | |
| """ | |
| settings: Dict[str, Any] = {"enabled": enabled} | |
| if enabled: | |
| criteria: Dict[str, Any] = {"type": criteria_type} | |
| if criteria_value is not None: | |
| if criteria_type == "completion" and criteria_value > 100: | |
| raise ValidationError( | |
| "Certificate criteria value cannot exceed 100 for completion type" | |
| ) | |
| criteria["value"] = criteria_value | |
| settings["criteria"] = criteria | |
| If enabled is False, returns None so that certificate settings can be | |
| omitted from the payload entirely. | |
| """ | |
| if not enabled: | |
| return None | |
| settings: Dict[str, Any] = {"enabled": True} | |
| criteria: Dict[str, Any] = {"type": criteria_type} | |
| if criteria_value is not None: | |
| if criteria_type == "completion" and criteria_value > 100: | |
| raise ValidationError( | |
| "Certificate criteria value cannot exceed 100 for completion type" | |
| ) | |
| criteria["value"] = criteria_value | |
| settings["criteria"] = criteria |
There was a problem hiding this comment.
Thanks for the suggestion. This behavior is intentional — certificate automation defaults to false, and sending {"enabled": false} does not change API behavior. The API validates the fields before processing, so if any unintended values are provided the request will fail. Because of this, always sending the field keeps the payload consistent without introducing side effects.
src/scripts/deploy_common.py
Outdated
| def validate_certificate_settings(value: str) -> Optional[Dict[str, Any]]: | ||
| """Validate and parse certificate settings JSON string.""" | ||
| if not value or not value.strip(): | ||
| return None | ||
| try: | ||
| settings = json.loads(value) | ||
| if not isinstance(settings, dict): | ||
| raise ValidationError("Certificate settings must be a JSON object") | ||
| if "enabled" in settings: | ||
| if not isinstance(settings["enabled"], bool): | ||
| raise ValidationError( | ||
| "Certificate settings 'enabled' must be a boolean" | ||
| ) | ||
| if "criteria" in settings: | ||
| criteria = settings["criteria"] | ||
| if not isinstance(criteria, dict): | ||
| raise ValidationError( | ||
| "Certificate settings 'criteria' must be an object" | ||
| ) | ||
| if "type" in criteria: | ||
| if criteria["type"] not in ["completion", "points"]: | ||
| raise ValidationError( | ||
| "Certificate criteria 'type' must be 'completion' or 'points'" | ||
| ) | ||
| if "value" in criteria: | ||
| if not isinstance(criteria["value"], (int, float)): | ||
| raise ValidationError( | ||
| "Certificate criteria 'value' must be a number" | ||
| ) | ||
| if criteria["type"] == "completion" and criteria["value"] > 100: | ||
| raise ValidationError( | ||
| "Certificate criteria 'value' cannot exceed 100 for completion type" | ||
| ) | ||
| if "templateId" in settings: | ||
| if settings["templateId"] and not isinstance(settings["templateId"], str): | ||
| raise ValidationError( | ||
| "Certificate settings 'templateId' must be a string" | ||
| ) | ||
| return settings | ||
| except json.JSONDecodeError as e: | ||
| raise ValidationError(f"Invalid JSON in certificate settings: {e}") | ||
|
|
||
|
|
||
| def validate_certificate_criteria_type(value: str) -> str: | ||
| """Validate certificate criteria type.""" | ||
| if not value or not value.strip(): | ||
| return "completion" | ||
| value = value.strip().lower() | ||
| if value not in ["completion", "points"]: | ||
| raise ValidationError( | ||
| "Certificate criteria type must be 'completion' or 'points'" | ||
| ) | ||
| return value | ||
|
|
||
|
|
||
| def validate_certificate_criteria_value(value: str) -> Optional[float]: | ||
| """Validate certificate criteria value.""" | ||
| if not value or not value.strip(): | ||
| return None | ||
| try: | ||
| val = float(value) | ||
| if val < 0: | ||
| raise ValidationError("Certificate criteria value must be non-negative") | ||
| return val | ||
| except ValueError: | ||
| raise ValidationError("Certificate criteria value must be a number") | ||
|
|
||
|
|
||
| def build_certificate_settings( | ||
| enabled: bool, | ||
| criteria_type: str, | ||
| criteria_value: Optional[float], | ||
| ) -> Optional[Dict[str, Any]]: | ||
| """Build certificate settings dict from individual fields. | ||
|
|
||
| If enabled is False, returns settings with enabled=False. | ||
| """ | ||
| settings: Dict[str, Any] = {"enabled": enabled} | ||
|
|
||
| if enabled: | ||
| criteria: Dict[str, Any] = {"type": criteria_type} | ||
| if criteria_value is not None: | ||
| if criteria_type == "completion" and criteria_value > 100: | ||
| raise ValidationError( | ||
| "Certificate criteria value cannot exceed 100 for completion type" | ||
| ) | ||
| criteria["value"] = criteria_value | ||
| settings["criteria"] = criteria | ||
|
|
||
| return settings |
There was a problem hiding this comment.
The new certificate-related functions (validate_certificate_settings, validate_certificate_criteria_type, validate_certificate_criteria_value, build_certificate_settings) and the updated get_common_payload logic that includes certificateSettings in the API payload have no test coverage. Given that test_deploy_common.py and test_create_course.py already cover related functionality, tests should be added to cover at least: valid certificate settings with enabled=True and enabled=False, invalid criteria type/value inputs, the KeyError edge case in validate_certificate_settings (value present without type), and that get_common_payload correctly includes or excludes certificateSettings.
src/scripts/common.py
Outdated
| # DEFAULT_API_BASE_URL: str = "https://api-v2.qbraid.com/api/v1" | ||
| DEFAULT_API_BASE_URL: str = "https://591a-117-213-50-18.ngrok-free.app/app1/api/v1" | ||
| API_BASE_URL: str = os.getenv("QBRAID_API_BASE_URL", DEFAULT_API_BASE_URL) | ||
| REQUEST_TIMEOUT_SECONDS: int = _get_env_int("QBRAID_REQUEST_TIMEOUT_SECONDS", 30) | ||
| MAX_POLL_ATTEMPTS: int = _get_env_int("QBRAID_MAX_POLL_ATTEMPTS", 20) |
There was a problem hiding this comment.
The production API base URL has been replaced with a local ngrok tunnel URL. This would route all API calls from any user of this action to a developer's local machine instead of the production qBraid API. This must be reverted to the original production URL before merging.
The DEFAULT_API_BASE_URL should be restored to "https://api-v2.qbraid.com/api/v1" and the ngrok URL (which appears to be a private/temporary tunnel) should be removed.
| # DEFAULT_API_BASE_URL: str = "https://api-v2.qbraid.com/api/v1" | |
| DEFAULT_API_BASE_URL: str = "https://591a-117-213-50-18.ngrok-free.app/app1/api/v1" | |
| API_BASE_URL: str = os.getenv("QBRAID_API_BASE_URL", DEFAULT_API_BASE_URL) | |
| REQUEST_TIMEOUT_SECONDS: int = _get_env_int("QBRAID_REQUEST_TIMEOUT_SECONDS", 30) | |
| MAX_POLL_ATTEMPTS: int = _get_env_int("QBRAID_MAX_POLL_ATTEMPTS", 20) | |
| DEFAULT_API_BASE_URL: str = "https://api-v2.qbraid.com/api/v1" | |
| API_BASE_URL: str = os.getenv("QBRAID_API_BASE_URL", DEFAULT_API_BASE_URL) | |
| REQUEST_TIMEOUT_SECONDS: int = _get_env_int("QBRAID_REQUEST_TIMEOUT_SECONDS", 30) | |
| MAX_POLL_ATTEMPTS: int = _get_env_int("QBRAID_MAX_POLL_ATTEMPTS", 20) | |
| MAX_POLL_ATTEMPTS: int = _get_env_int("QBRAID_MAX_POLL_ATTEMPTS", 20) |
src/scripts/deploy_common.py
Outdated
| raise ValidationError( | ||
| "Certificate criteria 'value' must be a number" | ||
| ) | ||
| if criteria["type"] == "completion" and criteria["value"] > 100: |
There was a problem hiding this comment.
In validate_certificate_settings, when criteria["value"] is present but criteria["type"] is not present in the JSON, accessing criteria["type"] on line 137 will raise a KeyError instead of the expected ValidationError. The "type" key presence check earlier (line 127) only validates the value of type if it exists, but the check at line 137 unconditionally reads criteria["type"] when criteria["value"] is found.
A "type" in criteria guard should be added before accessing criteria["type"] on line 137, or the check should use criteria.get("type") with a safe default.
| if criteria["type"] == "completion" and criteria["value"] > 100: | |
| if criteria.get("type") == "completion" and criteria["value"] > 100: |
…r CourseUpdater and CourseDeployer
This pull request adds support for certificate issuance settings to the course deployment GitHub Action. It introduces new input fields and validation logic for enabling certificates, specifying criteria type (completion or points), and setting the required value. The documentation and example workflows are updated to reflect these changes, and the certificate settings are now included in the course deployment payload when applicable.
Certificate Issuance Feature:
Added new inputs to the GitHub Action for certificate issuance:
certificate-enabled,certificate-criteria-type, andcertificate-criteria-value, with validation and default values. These allow users to configure whether a course issues certificates, and under what criteria (e.g., completion percentage or points). (action.yml,src/scripts/create_course.py,src/scripts/update_course.py,src/scripts/deploy_common.py) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17]Implemented certificate settings validation and construction logic, ensuring correct types and value ranges (e.g., completion cannot exceed 100%). (
src/scripts/deploy_common.py)Updated the deployment payload to include certificate settings only for articles of type
course. (src/scripts/deploy_common.py)Documentation and Examples:
Updated documentation (
README.md,AGENTS.md) to describe the new certificate-related inputs, their defaults, and provided usage examples for both completion-based and points-based certificates. [1] [2] [3]Added the new certificate inputs to example workflow files for clarity. (
examples/example.yaml,examples/workflow.yml) [1] [2]Other:
src/scripts/common.pyfor development/testing purposes.