Repository navigation
Conversation
…le edits
The user-edit form's email-notification checkbox stopped rendering after
the CKAN 2.11 upgrade: it gated on `show_email_notifications`, a template
variable CKAN removed in 2.10 in favour of
`h.activity_show_email_notifications()`. Point it at the current helper.
The checkbox's checked state read `g.userobj` (the logged-in user) instead
of `data` (the profile being edited) - copied from CKAN 2.9's own template,
which had the same bug. On any sysadmin save of another user's profile this
silently overwrote that user's real preference with the sysadmin's own.
Fixed to match upstream CKAN, which corrected this when the field moved
into ckanext-activity.
The checkbox also rendered stacked above its label: `.dataset-form input
{ width: 100% }` stretched it to the field's full width. Exempt checkboxes
and radios from that rule.
Separately, saving any change to another user's profile as a sysadmin
always failed with a misleading "Sysadmin Password: incorrect password"
error. `ckan/views/user.py` requires the acting sysadmin's own password to
authorize a cross-user edit, but this form always submitted that field
hidden and empty. Restore a visible, required password field for that case
only, matching CKAN's own current template.
data.oldpassword (no underscore) is never set anywhere in CKAN - the submitted form key is old_password, and user_show never returns password fields. Confirmed the input always rendered value="" regardless; removing this argument is a no-op, verified by comparing rendered HTML before and after. Kept purely for parity with the same dead argument in CKAN's own core template.
…sabled activity_show_email_notifications comes from ckanext-activity, which this extension declares no dependency on. If activity is absent from ckan.plugins while unaids is enabled, the helper is undefined and calling it raises, returning a 500 on /user/edit. The previous show_email_notifications check degraded gracefully - an undefined variable is simply falsy - so this restores that behaviour. 'key' in h is safe: HelperAttributeDict subclasses dict, so __contains__ does not trigger the __missing__ that raises HelperError.
…assword Two regression tests for the fixes in this branch, both verified to fail when the corresponding fix is reverted: - the notification checkbox must reflect the user being edited, not the logged-in viewer. Reverting to g.userobj renders it unchecked for a subscribed target user, which is what let a sysadmin's save silently overwrite that user's preference. - the sysadmin password field must appear only when editing someone else. Reverting to the always-hidden input leaves cross-user saves unable to supply the password the view requires.
Strengthen the two regression tests so they cannot pass for the wrong reason, both verified against a deliberately broken template: - the checkbox test now renders a subscribed and an unsubscribed target, each viewed by an oppositely subscribed sysadmin. Hardcoding checked=True satisfied the previous single-direction assertion. - the password test now asserts type="password" and required on the matched input. A hidden input reusing the same id satisfied the previous id-substring check while restoring the original bug. Matching the bare checked attribute also avoids counting a future data-checked as a positive.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three faults in the user-edit form, all stemming from the CKAN 2.9 → 2.11 migration and a Dec 2022 crash fix - none crashed or logged anything, they just silently didn't work.
show_email_notifications, a variable CKAN removed in 2.10 (replaced byh.activity_show_email_notifications()). Always false on 2.11, regardless of config.checked=g.userobj...(the viewer) instead ofchecked=data...(the profile being edited) - a CKAN 2.9 bug we copied verbatim. On any sysadmin save of another user's profile this silently overwrote that user's real preference with the sysadmin's own. Upstream fixed this themselves when the field moved intockanext-activity; matched here.views/user.pyrequires the sysadmin's own password to authorize a cross-user edit, butold_passwordwas always submitted hidden and empty (added to stop a self-edit crash, untested for cross-user). Every such save failed with a misleading "incorrect password" error. Restored a visible, required password field for that case only, matching CKAN's own current template.Also fixed: the checkbox rendered stacked above its label (
.dataset-form input { width: 100% }stretched it) - exempted checkboxes/radios from that rule.Second commit drops a dead
value=data.oldpasswordargument - verified that key is never set anywhere in CKAN, so it always renderedvalue=""regardless; removing it is a no-op (confirmed by diffing rendered HTML).Operational note (not part of this diff): enabling the config also needs something to trigger sending on a schedule. A cron job authenticating via a sysadmin API token works, but CKAN 2.11 tokens never expire - a permanent credential to manage. Running
ckan notify send_emailsin-process (e.g. via supervisor) needs no token at all. Recommend the latter wherever this gets scheduled; not blocking for this PR.Test plan
old_passwordhidden/empty - no behavior changevalue=argument.dataset-formapp-wide checked - only this one affected by the CSS change