OpenConceptLab/ocl_online#81 | Using AI Assistant to generate change comment in concept#25
OpenConceptLab/ocl_online#81 | Using AI Assistant to generate change comment in concept#25snyaggarwal wants to merge 6 commits into
Conversation
paynejd
left a comment
There was a problem hiding this comment.
Review changes as discussed — most are inline suggestions you can commit directly. Two items can't be expressed as suggestions and are noted below.
1. New i18n keys (can't suggest — translations.json isn't in this PR's diff)
Add to src/i18n/locales/en/translations.json (and es/zh, or they'll fall back to en):
// "common": { ...
"generate_with_ai": "Generate with AI",
// "concept": { ...
"ai_assistant_not_configured": "AI assistant is not configured for this environment.",
"make_change_before_generating": "Make a change to the concept before generating a comment.",
"generate_comment_aria": "Generate comment with AI",
"try_again_in_a_moment": "Try again in a moment."2. Backend auth (context, not a code change here)
Server-side, POST /prompts/{key}/$invoke/ calls check_auth_group(MAPPER_AI_ASSISTANT_GROUP) (mapper_ai_assistant) — it is not superuser-gated. So the front-end visibility gate (superuser / core_user) and the back-end permission (mapper_ai_assistant) are different groups. That's the intended "safe even if it doesn't yet work for everyone" state — worth a follow-up ticket to align the groups before opening this to all users.
paynejd
left a comment
There was a problem hiding this comment.
Reviewed against ocl_online#81. Overall a clean, faithful implementation — env-var plumbing mirrors the existing ANALYTICS_API pattern, token forwarding works via currentUserToken(), metadata-exclusion lists match the spec, and the loading/disabled/tooltip states cover the acceptance criteria. A few inline notes below, plus one cross-repo dependency on backend permissions.
| const { conceptClasses, datatypes, locales, nameTypes, descriptionTypes, fields } = this.state | ||
| const { conceptClasses, datatypes, locales, nameTypes, descriptionTypes, fields, generatingChangeComment } = this.state | ||
| const aiAssistantConfigured = Boolean(this.getAIAssistantURL()) | ||
| const canSeeGenerateComment = edit && (isSuperuser() || hasAuthGroup(getCurrentUser(), 'core_user')) |
There was a problem hiding this comment.
This gates the button to core_users in addition to superusers, but per the ticket the AI Assistant $invoke endpoint still requires superuser — it notes this "must be relaxed to allow staff/authenticated users before this feature ships (separate ticket needed)." @snyaggarwal could you take the backend permissions work so core_users can actually invoke this? Otherwise non-superuser core_users will see the button and hit a 403.
There was a problem hiding this comment.
Good catch, raising separate PR
There was a problem hiding this comment.
Co-authored-by: Jonathan Payne <paynejd@gmail.com>
Co-authored-by: Jonathan Payne <paynejd@gmail.com>
paynejd
left a comment
There was a problem hiding this comment.
Final pass against OpenConceptLab/ocl_online#81 — approving.
All items from the prior two reviews are addressed:
- Prompt key
concept-generate-change-comment+ canonical/$invoke/route - Response read via
data.outputonly - i18n keys added across
en/es/zh(incl.generic_errorfallbacks) - Visibility gate,
hasConceptChanges()computed once, button rendered only for permitted users update_commentpersistence fix inhandleSubmit(this is also what makes update comments persist at all)- eslint disable scoped to a single line
Also verified the mechanics: token forwarding works (request(..., null, ...) falls back to currentUserToken() in getHeaders), getValues() returns a fresh object so the delete calls don't mutate state, and the file is lint-clean (browser env + /*global process*/). Snyk green.
Merge/deploy order: this depends on the sibling backend PR OpenConceptLab/ocl-ai-assistant#187. Ship #187 to each environment before (or with) this one — until $invoke is relaxed, a superuser/core_user who isn't in mapper_ai_assistant will see the button and hit a 403.
Two optional, non-blocking nits:
- The empty-output guard throws a hardcoded English
'No generated comment was returned.'that can reach the user via the catch'serror.messagebranch — could fall back tot('common.generic_error'). Edge case only. - Pre-existing pattern: a Django superuser created outside
set_groups(nosuperadmin_usergroup) would see the button and 403 — degrades gracefully to a toast.
Linked Issue
Refs: OpenConceptLab/ocl_online#81