service: add CRUD actions for CheckConfigService #14
Conversation
| def validate_community_id(self, value): | ||
| """Validate that the community_id is a valid UUID.""" | ||
| try: | ||
| UUID(value) |
There was a problem hiding this comment.
minor: it might be nice to check if the community exist. It could also be that it is the caller checking that.
CommunityMetadata.query(CommunityMetadata.id == value).exists()
Just noticed that there is a foreign key.
| try: | ||
| validated_data = schema.load(data) | ||
| except ValidationError as e: | ||
| raise ValueError(f"Validation error: {e.messages}") |
There was a problem hiding this comment.
If there will be a form to create this, we will need to return a more structured error message, so that the UI can parse the response.
| enabled=validated_data.get("enabled", True), | ||
| ) | ||
| uow.register(ModelCommitOp(check_config)) | ||
| return check_config |
There was a problem hiding this comment.
We normally return a ResultItem, see for example here
| ) | ||
| uow.register(ModelCommitOp(check_config)) | ||
| return check_config | ||
| except Exception as e: |
There was a problem hiding this comment.
I would avoid, whenever possible, have a catch-all Exception, which can hide inner issues and makes it difficult for the caller to handle a specific exception. Is it needed?
| raise NotImplementedError() | ||
| self.require_permission(identity, "read") | ||
| check_config = get_check_config(id_) | ||
| return check_config |
| if "enabled" in params: | ||
| query = query.filter_by(enabled=params["enabled"]) | ||
|
|
||
| return query.all() |
There was a problem hiding this comment.
See ResultsList here. query.all() might be very inefficient without a limit or pagination, in case of a large number of results.
| return check_config | ||
| except NoResultFound: | ||
| raise ValueError(f"Check configuration with id '{id_}' not found") | ||
| except Exception as e: | ||
| raise ValueError(f"Failed to update check configuration: {str(e)}") |
|
|
||
| return True | ||
| except NoResultFound: | ||
| raise ValueError(f"Check configuration with id '{id_}' not found") | ||
| except Exception as e: | ||
| raise ValueError(f"Failed to delete check configuration: {str(e)}") |
❤️ Thank you for your contribution!
close #10
Description
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: