Refactor message handling to use invitation_group_id #1485

axif0 · 2025-03-23T19:11:23Z

Updated MessagesController and related views to replace group_id with invitation_group_id for better clarity and functionality.
Adjusted message_params to permit the new attribute and modified the schema to reflect this change. Ensured consistency across the application by updating references in the nh_messages_controller and views.

Related issue
#1037

prioux

Thanks for your contribution!

The purpose of the pull request is really to distinguish the attribute "group_id" in the Message object from all other uses of "group_id" in other contexts.

The group_id attribute in Message is not related at all to the message creation interface. It is used by the "group invitation" mechanism, where a message contains the group_id of the Group that people are invited to join. That is why I wanted it renamed.

So the pieces that are missing are in the Invitation model, where the group_id is being actually used to make invitations.

The remaining files that need to be adjusted are in

app/models/invitation.rb
app/controllers/invitations_controller.rb
app/controllers/nh_invitations_controller.rb

In particular, care should be taken to adjust the active record relationship directive belongs_to :group in the model, given by convention it makes a link between tables using "group_id <-> Group". Then in the controller code, the method to go throught he relationship shouls also be adjusted (e.g. line 96 of invitations_controller.rb)

prioux · 2025-03-24T13:21:37Z

BrainPortal/db/migrate/20120518181249_add_group_id_and_type_and_active_to_messages.rb

@@ -1,13 +0,0 @@
-class AddGroupIdAndTypeAndActiveToMessages < ActiveRecord::Migration


This file should not be deleted or modified (see my general comments for more info)

prioux · 2025-03-24T13:28:15Z

BrainPortal/db/migrate/20240323000000_rename_message_group_id_to_invitation_group_id.rb

@@ -0,0 +1,9 @@
+class RenameMessageGroupIdToInvitationGroupId < ActiveRecord::Migration[5.0]


I think this migration's code is fine, but I feel you created it manually instead of using the rails generator ("rails generate migration DoSomethingOrOther" generates the template for the file). I can tell because the name of the file starts with "20240323000000" which is definitely an artificial timestamp. It should starts with 202503 and the main schema.rb should have the same ID recorded in its version (line 13 of schema.rb).

All of this is generally done automatically by creating and running the migration on your local dev database. Please make the adjustments.

I feel you created it manually instead of using the rails generator

Ya, I created it manually. I’ll regenerate it using the Rails generator to ensure the correct timestamp and update the schema accordingly. Thanks for pointing it out!

prioux · 2025-03-24T13:29:33Z

BrainPortal/db/migrate/20120518181249_add_invitaion_group_id_and_type_and_active_to_messages.rb

@@ -0,0 +1,13 @@
+class AddInvitationGroupIdAndTypeAndActiveToMessages < ActiveRecord::Migration


I don't understand where this migration comes from, especially since it's from 2012. See my general comments.

I don't understand where this migration comes from, especially since it's from 2012. See my general comments.

Actually, I had renamed the file, which caused the confusion. I’ve reverted the files now.

prioux · 2025-03-24T13:30:44Z

BrainPortal/db/schema.rb

@@ -247,7 +247,7 @@
    t.datetime "last_sent"
    t.boolean  "critical",                    default: false, null: false
    t.boolean  "display",                     default: false, null: false
-    t.integer  "group_id"


This schema.rb's version has not been updated to reflect the applied migration (see my comment about the slightly wrong migration 20240323000000)

prioux · 2025-03-24T13:56:23Z

BrainPortal/app/controllers/messages_controller.rb

@@ -59,7 +59,7 @@ def index #:nodoc:

  def new #:nodoc:
    @message  = Message.new # blank object for new() form.
-    @group_id = nil         # for new() form
+    @invitation_group_id = nil         # for new() form


I don't think this file requires any changes at all. See my general comments.

prioux · 2025-03-24T14:01:51Z

BrainPortal/app/models/message.rb

@@ -74,7 +75,7 @@ def self.send_message(destination, options = {})
    expiry       = options[:expiry]        || options["expiry"]
    critical     = options[:critical]      || options["critical"]      || false
    send_email   = options[:send_email]    || options["send_email"]    || false
-    group_id     = options[:group_id]      || options["group_id"]
+    group_id     = options[:group_id]      || options["group_id"]      # For invitation messages only.


So we should rename the variable and the key in the options hash to 'invitation_group_id' and make sure that the change is applied in places where this is called (which should only be when making Invitation objects)

prioux · 2025-03-24T14:02:46Z

BrainPortal/app/views/messages/new.html.erb

@@ -44,10 +44,10 @@
     </p>

     <p>
-     To users of project: <%= group_select(:group_id,
+     To users of project: <%= group_select(:destination_group_id,


No changes needed in this file, this group_id is again the group of people to send a message to, not an invitation group ID.

prioux · 2025-03-24T14:03:24Z

BrainPortal/app/views/messages/new_dashboard.html.erb

@@ -44,8 +44,8 @@
      ] %>
    </p>

-    <% # group_id is not the attribute of the Message object %>
-    <%= hidden_field_tag :group_id, current_user.own_group.id %>
+    <% # destination_group_id is not the attribute of the Message object %>


This should also stay as "group_id"

prioux · 2025-03-24T14:13:04Z

As an additional comment: yes I'm fully aware this is a tricky change to get right, we're working with some legacy code that originally was not using 'good' names for the invitation framework.

prioux · 2025-03-24T14:20:15Z

Also @axif0 let me know if you decide not to adjust the PR, I will then make the changes myself because I feel bad about creating the original issue without having described the associated technical challenges.

prioux · 2025-03-24T14:59:46Z

(I just made some edits to my messages because some of the sentences were moved from local code comments to the main review comments, and made no sense anymore)

…d in controller and views

axif0 · 2025-03-24T17:03:00Z

I updated rename_message_group_id_to_invitation_group_id as

If neither column exists, it will add the appropriate one
If the old column exists but new doesn't, it will perform the rename

Also, thanks for the detailed elaboration.

prioux · 2025-03-24T17:18:24Z

BrainPortal/db/migrate/20250324164153_rename_message_group_id_to_invitation_group_id.rb

@@ -0,0 +1,27 @@
+class RenameMessageGroupIdToInvitationGroupId < ActiveRecord::Migration[5.0]


All these checks are unnecessary. The migration you had in the previous commit was already perfectly fine. There is no need to check for the column and create it if needed, because when the migration is run, it is guaranteed that the column will be present. Just revert to the what it was:

def up rename_column :messages, :group_id, :invitation_group_id end def down rename_column :messages, :invitation_group_id, :group_id end

Refactor message handling to use invitation_group_id

fe5af70

prioux requested changes Mar 24, 2025

View reviewed changes

Update message handling to use group_id instead of invitation_group_i…

3e2e11e

…d in controller and views

axif0 requested a review from prioux March 24, 2025 17:03

prioux reviewed Mar 24, 2025

View reviewed changes

revert migration checks

8b4ddf0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor message handling to use invitation_group_id #1485

Refactor message handling to use invitation_group_id #1485

axif0 commented Mar 23, 2025

prioux left a comment •

edited

Loading

prioux Mar 24, 2025

prioux Mar 24, 2025

axif0 Mar 24, 2025

prioux Mar 24, 2025

axif0 Mar 24, 2025

prioux Mar 24, 2025

prioux Mar 24, 2025

prioux Mar 24, 2025

prioux Mar 24, 2025

prioux Mar 24, 2025

prioux commented Mar 24, 2025

prioux commented Mar 24, 2025

prioux commented Mar 24, 2025

axif0 commented Mar 24, 2025

prioux Mar 24, 2025 •

edited

Loading

		@@ -1,13 +0,0 @@
		class AddGroupIdAndTypeAndActiveToMessages < ActiveRecord::Migration

		@@ -0,0 +1,9 @@
		class RenameMessageGroupIdToInvitationGroupId < ActiveRecord::Migration[5.0]

		@@ -0,0 +1,13 @@
		class AddInvitationGroupIdAndTypeAndActiveToMessages < ActiveRecord::Migration

		@@ -0,0 +1,27 @@
		class RenameMessageGroupIdToInvitationGroupId < ActiveRecord::Migration[5.0]

Refactor message handling to use invitation_group_id #1485

Are you sure you want to change the base?

Refactor message handling to use invitation_group_id #1485

Conversation

axif0 commented Mar 23, 2025

prioux left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prioux commented Mar 24, 2025

prioux commented Mar 24, 2025

prioux commented Mar 24, 2025

axif0 commented Mar 24, 2025

prioux Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

prioux left a comment •

edited

Loading

prioux Mar 24, 2025 •

edited

Loading