Skip to content

Revert "Revert "refactor(aichat): move request controller helpers to chat helpers""#70368

Merged
drizco merged 3 commits into
stagingfrom
revert-70366-revert-70249-ryan/aichat/refactor/move-request-controller-helpers-to-chat-helpers
Jan 20, 2026
Merged

Revert "Revert "refactor(aichat): move request controller helpers to chat helpers""#70368
drizco merged 3 commits into
stagingfrom
revert-70366-revert-70249-ryan/aichat/refactor/move-request-controller-helpers-to-chat-helpers

Conversation

@drizco

@drizco drizco commented Jan 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Reverts #70366

reverts a revert for #70249 where I added strong parameters to the aichat request controller, but missed a few

instead of updating the permitted params, I'm permitting all like it was before. I've created a ticket to follow up and add strong parameters in a future pr https://codedotorg.atlassian.net/browse/TEACHING-60

@drizco drizco force-pushed the revert-70366-revert-70249-ryan/aichat/refactor/move-request-controller-helpers-to-chat-helpers branch from 7871df5 to ac5747e Compare January 16, 2026 17:14
@drizco drizco marked this pull request as ready for review January 16, 2026 17:22
cnbrenci
cnbrenci reviewed Jan 20, 2026
View reviewed changes
Comment thread dashboard/app/controllers/aichat_requests_controller.rb Outdated
end

def create_request
safe_params = params.permit(

@cnbrenci cnbrenci Jan 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think it'd be helpful to put a few comments calling out the omitted params in this structure and why they were omitted

@drizco drizco Jan 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and removed the strong parameters, but we'll add them back in this ticket https://codedotorg.atlassian.net/browse/TEACHING-60

@edcodedotorg

edcodedotorg commented Jan 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

this commit fix: add missing params for aichat request should contain all the params that are sent from the front end and are used in the back end. (notably I excluded chatMessageDisplayText, hiddenContext, and userAddedSelectionContext from storedMessages as they're not used. and I removed
this commit fix: add missing params for aichat request should contain all the params that are sent from the front end and are used in the back end. (notably I excluded chatMessageDisplayText, hiddenContext, and userAddedSelectionContext from storedMessages as they're not used. and I removed updateId from both the newMessage and storedMessages since that's specifically for the front end.)

Hey, taking a quick look and I'm trying to understand what is meant by "I excluded chatMessageDisplayText, hiddenContext, and userAddedSelectionContext from storedMessages as they're not used.".

Focusing on e.g. chatMessageDisplayText, by "not used" do you mean this is not needed at all in this controller (vs. not needed in the AichatRequest model we're creating with the filtered params) even though it is needed in AichatEventsController which is currently called from the frontend with the same types?

I think we might benefit from separating the introduction of Strong Parameters into a separate PR (like you mentioned in check-in, removing the .permit from this PR, but with the aim to add it back), as I think it's important, but also important to get right. This would be in line with the Just Right PR Size ideas.

I think we should be using .permit but also want to make sure we clean up any front end code that is sending superfluous params. But definitely your call.

@drizco

drizco commented Jan 20, 2026

Copy link
Copy Markdown
Contributor Author

I think we might benefit from separating the introduction of Strong Parameters into a separate PR (like you mentioned in check-in, removing the .permit from this PR, but with the aim to add it back), as I think it's important, but also important to get right. This would be in line with the Just Right PR Size ideas.

totally agree, will do!

@drizco drizco force-pushed the revert-70366-revert-70249-ryan/aichat/refactor/move-request-controller-helpers-to-chat-helpers branch from ac5747e to 4ca442c Compare January 20, 2026 21:04
@drizco drizco requested a review from cnbrenci January 20, 2026 21:30
edcodedotorg
edcodedotorg approved these changes Jan 20, 2026
View reviewed changes

@edcodedotorg edcodedotorg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@drizco drizco merged commit cf9e5e4 into staging Jan 20, 2026
6 checks passed
@drizco drizco deleted the revert-70366-revert-70249-ryan/aichat/refactor/move-request-controller-helpers-to-chat-helpers branch January 20, 2026 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edcodedotorg edcodedotorg edcodedotorg approved these changes

@bencodeorg bencodeorg Awaiting requested review from bencodeorg

@fisher-alice fisher-alice Awaiting requested review from fisher-alice

@Erin007 Erin007 Awaiting requested review from Erin007

@molly-moen molly-moen Awaiting requested review from molly-moen

@sanchitmalhotra126 sanchitmalhotra126 Awaiting requested review from sanchitmalhotra126

@cnbrenci cnbrenci Awaiting requested review from cnbrenci

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@drizco @edcodedotorg @cnbrenci