Skip to content

implementation/60980 - Define restricted extension on work package journals for query safety#18366

Merged
akabiru merged 2 commits intoimplementation/60980-introduce-work-package-restricted-comments-as-part-of-journal-notesfrom
implementation/60980-use-ar-extensions-for-safety
Mar 20, 2025
Merged

implementation/60980 - Define restricted extension on work package journals for query safety#18366
akabiru merged 2 commits intoimplementation/60980-introduce-work-package-restricted-comments-as-part-of-journal-notesfrom
implementation/60980-use-ar-extensions-for-safety

Conversation

@akabiru
Copy link
Copy Markdown
Member

@akabiru akabiru commented Mar 19, 2025

AR association extensions ensure the restricted_visible scope is only visible on the work package journals association.

#18091 (comment)

@akabiru akabiru marked this pull request as draft March 19, 2025 06:05
@akabiru akabiru force-pushed the implementation/60980-use-ar-extensions-for-safety branch from 05cd97c to ebf79ea Compare March 19, 2025 06:11
@akabiru akabiru marked this pull request as ready for review March 19, 2025 06:13
@akabiru akabiru requested review from brunopagno and ulferts March 19, 2025 06:13
@akabiru
Copy link
Copy Markdown
Member Author

akabiru commented Mar 19, 2025

Oops- forgot to update the callers. 🏃🏾

@akabiru akabiru force-pushed the implementation/60980-use-ar-extensions-for-safety branch from ebf79ea to 1b28e63 Compare March 19, 2025 06:23
Copy link
Copy Markdown
Contributor

@brunopagno brunopagno left a comment

Choose a reason for hiding this comment

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

The code changes look okay to me.

I do feel it's going to be more difficult for someone to find the restricted_visible method when someone wishes to debug it, but it's a bit of the nature of rails applications.

But it seems to have broken some specs, so we need to look into those before pushing forward

@akabiru akabiru force-pushed the implementation/60980-use-ar-extensions-for-safety branch 2 times, most recently from fb377ac to b6f6812 Compare March 19, 2025 10:45
  AR association extensions ensure the `restricted_visible` scope is only visible on
  the work package journals association.

  #18091 (comment)
@akabiru akabiru force-pushed the implementation/60980-use-ar-extensions-for-safety branch from b6f6812 to 078c326 Compare March 19, 2025 10:45
@akabiru
Copy link
Copy Markdown
Member Author

akabiru commented Mar 19, 2025

The code changes look okay to me.

I do feel it's going to be more difficult for someone to find the restricted_visible method when someone wishes to debug it, but it's a bit of the nature of rails applications.

Cheers for the review- tbf, finding the association was already hard enough- and the journalization is encapsulated in one mixin, so hopefully that and the unit tests improve the odds of working it out. Definitely open to ideas on improving this 👍🏾

But it seems to have broken some specs, so we need to look into those before pushing forward

yeah, I blindly enabled frozen string literals - which broke some existing string mutations that I'd rather not mess with in this PR.

@akabiru akabiru requested a review from brunopagno March 19, 2025 11:06
Copy link
Copy Markdown
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

Overall, I like this change @akabiru . To me, it reads nicer and appears to be more solid. Thanks for picking up my review feedback from before.

I added on point that I think could improve readability but as is the nature of such things, it is opinionated. So feel free to ignore it if you disagree.

@akabiru akabiru requested a review from ulferts March 20, 2025 06:51
Copy link
Copy Markdown
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

👍

@akabiru akabiru merged commit f794f26 into implementation/60980-introduce-work-package-restricted-comments-as-part-of-journal-notes Mar 20, 2025
9 checks passed
@akabiru akabiru deleted the implementation/60980-use-ar-extensions-for-safety branch March 20, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants