ROCK-8438 Update obsolete code in CMS plugin blocks#229
Open
stphnlee wants to merge 4 commits into
Open
Conversation
Contributor
stphnlee
commented
May 19, 2026
stphnlee
commented
- CacheTagsCheckList: Get/SetUserPreference -> GetGlobalPersonPreferences().GetValue/SetValue + Save
- ContactGroupLeaders: DefinedValueField/SMSFromDefinedValueId -> SystemPhoneNumberField/SmsFromSystemPhoneNumberId via SystemPhoneNumberCache
- LoginStatus: Get/SetUserPreference -> GetGlobalPersonPreferences().GetValue/SetValue + Save
- CacheTagsCheckList: Get/SetUserPreference -> GetGlobalPersonPreferences().GetValue/SetValue + Save - ContactGroupLeaders: DefinedValueField/SMSFromDefinedValueId -> SystemPhoneNumberField/SmsFromSystemPhoneNumberId via SystemPhoneNumberCache - LoginStatus: Get/SetUserPreference -> GetGlobalPersonPreferences().GetValue/SetValue + Save Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates SECC CMS plugin blocks to replace obsolete Rock APIs, mainly around user preferences and SMS “from” configuration, to align with newer Rock patterns.
Changes:
- Updated
LoginStatusandCacheTagsCheckListto usePersonPreferences-style APIs instead ofGetUserPreference/SetUserPreference. - Updated
ContactGroupLeadersto useSystemPhoneNumberFieldandSmsFromSystemPhoneNumberId(viaSystemPhoneNumberCache) instead of defined values for SMS origin numbers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Plugins/org.secc.Cms/org_secc/Cms/LoginStatus.ascx.cs | Replaces obsolete preference read/write calls for the dashboard “last viewed” timestamp. |
| Plugins/org.secc.Cms/org_secc/Cms/ContactGroupLeaders.ascx.cs | Migrates SMS “from” configuration to System Phone Numbers and updates the communication property accordingly. |
| Plugins/org.secc.Cms/org_secc/Cms/CacheTagsCheckList.ascx.cs | Replaces obsolete preference persistence for selected cache-tag checkboxes. |
Comments suppressed due to low confidence (2)
Plugins/org.secc.Cms/org_secc/Cms/LoginStatus.ascx.cs:213
- This reads "LastViewedDashboard" from global person preferences, which can cause cross-block collisions and changes behavior compared to the previous GetUserPreference (block-scoped). Consider reading from the corresponding block-scoped preference store to keep the notification logic consistent per block instance.
var lastChecked = GetGlobalPersonPreferences().GetValue( "LastViewedDashboard" ).AsDateTime();
Plugins/org.secc.Cms/org_secc/Cms/CacheTagsCheckList.ascx.cs:162
- This loads selected values from global person preferences, which can change behavior vs the previous GetUserPreference (block-scoped). Consider switching to block-scoped preferences so different instances of the block don’t share the same saved checkbox state.
protected void LoadPreferences()
{
string usersselectedvalues = GetGlobalPersonPreferences().GetValue( "checkbox-selected-values-preference" );
IEnumerable<string> setvalues = usersselectedvalues.Split( ',' ).ToList();
cbl.SetValues( setvalues );
Comment on lines
151
to
156
| protected void Save() | ||
| { | ||
| SetUserPreference( "checkbox-selected-values-preference", GetSelectedValues() ); | ||
| var preferences = GetGlobalPersonPreferences(); | ||
| preferences.SetValue( "checkbox-selected-values-preference", GetSelectedValues() ); | ||
| preferences.Save(); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
@copilot apply changes based on this feedback
Contributor
There was a problem hiding this comment.
Done. Switched both Save() and LoadPreferences() in CacheTagsCheckList.ascx.cs, and also the read in LoginStatus.ascx.cs line 213, from GetGlobalPersonPreferences() to GetBlockPersonPreferences(). This ensures preference values are scoped to each block instance rather than shared globally. Commit: c0b87c7
- ChangeRequests: Get/SetBlockUserPreference -> GetBlockPersonPreferences().GetValue/SetValue + Save Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ences() in CacheTagsCheckList and LoginStatus Agent-Logs-Url: https://github.com/secc/RockPlugins/sessions/19e03026-da6f-4401-80af-67e012919c7c Co-authored-by: stphnlee <902855+stphnlee@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
Plugins/org.secc.Cms/org_secc/Cms/CacheTagsCheckList.ascx.cs:162
LoadPreferences()assumes the stored preference string is non-null.GetBlockPersonPreferences().GetValue(...)can return null when the preference has never been set, which will throw aNullReferenceExceptionon.Split(','). Consider defaulting to an empty string (and/or usingStringSplitOptions.RemoveEmptyEntries) before splitting.
string usersselectedvalues = GetBlockPersonPreferences().GetValue( "checkbox-selected-values-preference" );
IEnumerable<string> setvalues = usersselectedvalues.Split( ',' ).ToList();
cbl.SetValues( setvalues );
Plugins/org.secc.Cms/org_secc/Cms/ContactGroupLeaders.ascx.cs:376
SmsFromSystemPhoneNumberIdis assigned with a null-conditional lookup. If the attribute GUID is missing/invalid (or the cache entry isn't found), this will persist a Communication with no from phone number, and the send transaction may fail. Consider validating the lookup result and surfacing a user-friendly error (or falling back to a known default) before saving/sending.
communication.SmsFromSystemPhoneNumberId = SystemPhoneNumberCache.Get( GetAttributeValue( "SMSFromNumber" ).AsGuid() )?.Id;
communication.SMSMessage = GetAttributeValue( "SMSMessageBody" );
Comment on lines
+153
to
+155
| var preferences = GetBlockPersonPreferences(); | ||
| preferences.SetValue( "checkbox-selected-values-preference", GetSelectedValues() ); | ||
| preferences.Save(); |
| [LavaCommandsField( "Enabled Lava Commands", "The Lava commands that should be enabled for this HTML block.", false, order: 15 )] | ||
| [BooleanField( "Enable SMS", "Enable messages being sent by SMS when it is the user's preferred communication method.", false, order: 16 )] | ||
| [DefinedValueField( "611bde1f-7405-4d16-8626-ccfedb0e62be", "SMS From Number", "The SMS Phone Number that the message should come from", false, false, "86604119-a222-4b35-9cd3-1a78db1b7b17", "", order: 17 )] | ||
| [SystemPhoneNumberField( "SMS From Number", "The SMS Phone Number that the message should come from", false, false, "86604119-a222-4b35-9cd3-1a78db1b7b17", "", order: 17 )] |
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.