[TAN-6658] Gate prescreening_mode effect on the feature flag, not the write#14272
[TAN-6658] Gate prescreening_mode effect on the feature flag, not the write#14272jinjagit wants to merge 6 commits into
Conversation
|
…ion_method conversion
| isScreeningEnabled={ | ||
| !!phase?.data.attributes.prescreening_mode | ||
| } | ||
| isScreeningEnabled={screeningEnabled} |
There was a problem hiding this comment.
Defensive fix — not reachable in the UI today. StatusFilterDropdown only renders when showDropdownFilters is true, which never happens on a project page (ideation/proposals take the IdeasWithFiltersSidebar branch, where the real fix is in StatusFilterBox), and the only other caller passes no phaseId.
Kept anyway because the old code (!!phase?.data.attributes.prescreening_mode) encoded the invariant this PR removes — that a mode implies the feature is on. Any future caller passing a phaseId would inherit the bug.
adessy
left a comment
There was a problem hiding this comment.
I guess the main question is: how should feature flag changes affect existing phases that use prescreening? That's the main difference here. In this PR, disabling the feature disables it for all phases, while the original design kept it active for phases already using it. Both approaches are defensible, but since feature flag downgrades rarely happen in practice, the discussion might be moot, and maybe technical considerations should take precedence over product decisions here.
Besides the product question above, here are a few things I spotted:
- The computation of the effective prescreening mode is duplicated between the frontend and the backend, and the frontend implementation doesn't seem to replicate the backend logic faithfully: for a phase with
prescreening_mode: 'flagged_only', the backend treats screening as off whenflag_inappropriate_contentis disabled, while the frontend hook only checks the prescreening flags. If we're sticking with the whole effective + raw mode approach, I think the backend should expose the effective mode through the serializer instead. - The
ScreeningStatusFiltercomponent in the admin input manager still uses the raw column instead of the effective mode. - Not from this PR, but the same component doesn't take into account whether any inputs actually have the prescreening status. We could consider keeping the filter active as long as there are inputs waiting to be screened, even if the feature is disabled. (Note that moderators still receive the digest email based on input counts, I believe.)
If we wanted to keep the original behaviour, another option would be to keep the validation and instead normalize the screening mode when importing a phase, along the lines of:
before_validation :drop_unavailable_prescreening_mode, if: -> { Current.loading_tenant_template }
def drop_unavailable_prescreening_mode
return if prescreening_mode.nil?
required_features = [participation_method == 'proposals' ? 'prescreening' : 'prescreening_ideation']
required_features << 'flag_inappropriate_content' if prescreening_mode == 'flagged_only'
unless required_features.all? { |f| AppConfiguration.instance.feature_activated?(f) }
self.prescreening_mode = nil
end
end(or in a before_save, if we'd rather keep the validation phase free of mutations). That said, some aspects of this approach are questionable too.
Kind of, but the issues that arise via templating (e.g. creating a tenant with a template from source tenant where feature is/was on and phases do use it) are also a key issue. This is not a feature flag change, but rather a feature flag mismatch between the phase in the template and the target new tenant. Whatever solution we use should, and probably will, cover both cases (disabling feature in a tenant vs creating a tenant with feature off from template with conflicting phase data) In any case, @adessy, you raise some good points. Thanks! I'll revisit this as soon as I get time, and ask for your re-review when I am happy I have addressed your points, etc. |
Problem
Issue first detected as an error when creating a tenant via AdminHQ from blank_demo template.
I cleared the problematic
prescreening_modeflag value on the phases where it was set on the source tenant, as a temporary fix.When I re-enabled the flag on a phase on the source tenant (to recreate the issue) the issue arose when validating the templates in the nightly CI template run. This suggests the problematic validation was introduced after the initial values were set for the phases in question.
I any case, the issues were from the same cause: Use of
AppConfiguration.instance.feature_activated?checks in the Phase model'svalidate :validate_prescreening_mode. Such an approach is often a bad idea for exactly this reason, and we generally try to avoid it.Solution
Gate the behaviour in the BE, not via the validity of the Phase model.
This means we can write any value into the
phases.prescreening_modecolumn, but only make use of it/them if the relevant features are active, thus repairing the templating system in regards to this field.An added bonus is that the solution here also avoids similar validation errors that could have occurred when copying projects between tenants (using the project copy tool).
I also needed to make a few changes to the FE to handle the fact that the serialised
phase.prescreening_modemay conflict with the related feature flags.See the Claude plan for lots more details (including audit of potential problematic data states on production): TAN-6658-fix-phase-validation-of-prescreening-mode-invalidating-template.md
Notes/caveats
The admin phase form still silently re-persists an invisible
prescreening_mode(phaseSetup/index.tsx:84). Not a new bug I introduced — but the consequence is new: if such a tenant is later upgraded and the flag switched on, screening activates on a phase no admin ever configured (or configured at a previous time when feature was on). I have deliberately left this (the data surviving is arguably the point, and in practice this edge case is unlikely to cause issues).The
proposals→ideationconversion edge: because the flag is now chosen at read time, converting a proposals phase to ideation on a default-configured platform silently deactivates screening (prescreeningdefaults on,prescreening_ideationdefaults off), where master kept it active. Only reachable on a phase with zero inputs, and the new behaviour is arguably correct — documented in a new a test.General local functional testing of the various prescreening features revealed no issues (details in functional testing section of Claude plan)
Changelog
Fixed
prescreening_modeeffect on the feature flag, not the write. This fixes case where could not create a tenant (a.k.a. 'platform') from template containing phase(s) withprescreening_modeset.