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

axif0 · 2025-04-03T19:58:55Z

@prioux Is there any changes that needs to be made for this PR?

Also, can you suggest some good first issue that needs to be done ?

prioux · 2025-04-03T20:05:25Z

I'll gave to review it. I didn't realize you updated it following my comments, sorry. I'll have a look tomorrow.

prioux · 2025-04-04T03:21:57Z

@axif0 Also you asked for a suggestion of something to do. The issue #849 is pretty easy. There's only one file to change, BrainPortal/app/model/cluster_task.rb, in the method capture_job_out_err(). The method reads four files and store the content inside four object attributes. Two of them (cluster_stdout and cluster_stderr) need some filtering before storing. We need to remove ANSI sequences, these are special byte sequences that can be recognized by matching. E.g. the byte sequence ESCAPE (ascii 27), followed by [, 0 and m ("\e[0m" is another way of expressing all four bytes) is the code to reset color in a terminal. There are hundreds of such sequences, instead of identifying each one, remove them all in one efficient scan of the file. We probably only need to remove the very common ones, for color, underline, bold, background color, etc. Suggestion:

  <esc> '['  <any number of digits or semicolons> <any single letter>

This would need to be validated with an official ANSI sequence document.

prioux

Small style comments only.

I have a feeling other parts of the code need changing, but I will now test the PR in my dev environement.

prioux · 2025-04-08T17:16:11Z

BrainPortal/db/migrate/20250324164153_rename_message_group_id_to_invitation_group_id.rb

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


Missing newline at the end of the file. Our convention in CBRAIN is that all lines must end with a newline, even the last one.

prioux · 2025-04-08T17:16:43Z

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

@@ -92,4 +92,4 @@
      <%= f.submit "Submit" %>
    </p>
  </div>
-<% end %>
+<% end %>


Missing newline

prioux · 2025-04-08T17:16:52Z

BrainPortal/db/migrate/20120518181249_add_group_id_and_type_and_active_to_messages.rb

@@ -10,4 +10,4 @@ def self.down
    remove_column :messages, :group_id
    remove_column :message,  :active
  end
-end
+end


Missing terminal newline

prioux · 2025-04-08T17:18:50Z

BrainPortal/app/controllers/messages_controller.rb

@@ -178,4 +178,4 @@ def destroy #:nodoc:
  def message_params
    params.require(:message).permit(:header, :description, :variable_text, :message_type, :read, :user_id, :expiry, :last_sent, :critical, :display, :send_email, :group_id, :sender_id)
  end
-end
+end


Missing newline

This actually means no changed are supposed to happen to messages_controller.rb I think

prioux · 2025-04-08T17:33:12Z

Yep, I got a crash at line 144 of BrainPortal/app/views/groups/show.html.erb

So there are other places that need to be adjusted. Please make sure you can invite users and accept the invitation with dummy users in your dev environment.

prioux · 2025-04-08T17:37:13Z

Also line 43 of invitations_controller.rb

prioux · 2025-04-08T17:50:23Z

Also no invitation messages are created because the parameter for the group ID is set with key :invitation_group_id while in the message.rb code, it is still expected to be :group_id

prioux · 2025-04-08T18:21:02Z

Currently I have three required changes:

diff --git a/BrainPortal/app/controllers/invitations_controller.rb b/BrainPortal/app/controllers/invitations_controller.rb
index 968b4085a..de2970ae5 100644
--- a/BrainPortal/app/controllers/invitations_controller.rb
+++ b/BrainPortal/app/controllers/invitations_controller.rb
@@ -40,7 +40,7 @@ class InvitationsController < ApplicationController
        return
     end
 
-    already_sent_to = Invitation.where(sender_id: current_user.id, active: true, group_id: @group.id).all.map(&:user_id)
+    already_sent_to = Invitation.where(sender_id: current_user.id, active: true, invitation_group_id: @group.id).all.map(&:user_id)
     @users = current_user.visible_users.where("users.id NOT IN (?)", @group.users.map(&:id) | already_sent_to)
     render :partial => "new"
   end

diff --git a/BrainPortal/app/models/message.rb b/BrainPortal/app/models/message.rb
index 6bd6b7aed..6bf05c252 100644
--- a/BrainPortal/app/models/message.rb
+++ b/BrainPortal/app/models/message.rb
@@ -75,7 +75,7 @@ class Message < ApplicationRecord
     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"]      # For invitation messages only.
+    group_id     = options[:invitation_group_id] || options["invitation_group_id"]
     sender_id    = options[:sender_id]     || options["sender_id"]
 
     # Affects how 'destination' is interpreted. Default: exclude locked users.

and

diff --git a/BrainPortal/app/views/groups/show.html.erb b/BrainPortal/app/views/groups/show.html.erb
index 325348692..8ee4f254e 100644
--- a/BrainPortal/app/views/groups/show.html.erb
+++ b/BrainPortal/app/views/groups/show.html.erb
@@ -141,7 +141,7 @@
 
   <%
     group_members  = @group.users
-    open_invites   = Invitation.where(sender_id: current_user.id, group_id: @group.id, active: true).to_a
+    open_invites   = Invitation.where(sender_id: current_user.id, invitation_group_id: @group.id, active: true).to_a
   %>
 
   <%= show_table(@group, :as => :group, :header => 'Members', :edit_condition => @group.can_be_edited_by?(current_user) && (!current_user.has_role?(:normal_user) || group_members.count > 1 || open_invites.count > 0 )) do |t| %>

… group_id

axif0 · 2025-04-08T19:32:29Z

Thank you so much for the detailed explanation! @prioux I really appreciate your guidance. I'm still learning Ruby and getting familiar with the codebase, so apologies if I miss anything along the way.

prioux · 2025-04-17T19:13:34Z

I think this PR is ready now. I will do a revision.

Small adjustments to axif0's pull request

axif0 · 2025-04-17T19:52:32Z

Thank you for the changes. @prioux

prioux · 2025-04-17T19:55:30Z

I'm going to merge as soon as the automated tests are finished.

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

prioux requested changes Apr 8, 2025

View reviewed changes

Update invitations and messages to use invitation_group_id instead of…

1d4409e

… group_id

prioux and others added 2 commits April 17, 2025 15:44

Small adjustments to axif0's pull request

99fb8bc

Merge pull request #1 from prioux/invitation_group_id

4fa8829

Small adjustments to axif0's pull request

prioux linked an issue Apr 17, 2025 that may be closed by this pull request

Rename Message attribute group_id to "invitation_group_id" #1037

Closed

prioux assigned axif0 Apr 17, 2025

prioux added Refactoring Priority: Normal labels Apr 17, 2025

prioux merged commit 39ebc8c into aces:master Apr 17, 2025
1 check passed

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

axif0 commented Apr 3, 2025

prioux commented Apr 3, 2025

prioux commented Apr 4, 2025 •

edited

Loading

prioux left a comment

prioux Apr 8, 2025

prioux Apr 8, 2025

prioux Apr 8, 2025

prioux Apr 8, 2025

prioux Apr 8, 2025

prioux commented Apr 8, 2025

prioux commented Apr 8, 2025

prioux commented Apr 8, 2025

prioux commented Apr 8, 2025

axif0 commented Apr 8, 2025

prioux commented Apr 17, 2025

axif0 commented Apr 17, 2025

prioux commented Apr 17, 2025

		@@ -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

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

axif0 commented Apr 3, 2025

prioux commented Apr 3, 2025

prioux commented Apr 4, 2025 • edited Loading

prioux left a 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 Apr 8, 2025

prioux commented Apr 8, 2025

prioux commented Apr 8, 2025

prioux commented Apr 8, 2025

axif0 commented Apr 8, 2025

prioux commented Apr 17, 2025

axif0 commented Apr 17, 2025

prioux commented Apr 17, 2025

prioux left a comment •

edited

Loading

prioux Mar 24, 2025 •

edited

Loading

prioux commented Apr 4, 2025 •

edited

Loading