Skip to content

Mass Update Refactor - #1940

Open
JasonBennett41 wants to merge 51 commits into
masterfrom
mass_update_refactor
Open

Mass Update Refactor#1940
JasonBennett41 wants to merge 51 commits into
masterfrom
mass_update_refactor

Conversation

@JasonBennett41

Copy link
Copy Markdown

This PR was branched off of #1915 which is still pending review along with #1910. This means the blame for some of the modifications in 'Files Changed' is on the preceding PRs. Once the preceding PRs are approved, will probably have to merge master into this branch again.

Changes:

  • Refactored mass_update actions for ResourceScoresController & ResourceDefaultOrdersController
  • Refactored mass_update_ranked action in ResourceScoresController
  • Added support for field params for featured & default_order in ResourcesController
  • Added unique_position_per_language_and_resource_type validation in ResourceDefaultOrder model
  • Decremented MAX_FEATURED_ORDER_POSITION to align with frontend expectations in ResourceScore model
  • Changed featured_has_order_assigned validation to featured_and_featured_order_consistency -- a bidirectional equivalent -- in ResourceScore model
  • Modified specs to account for changes in the models

@stage-branch-merger

Copy link
Copy Markdown

Merge conflict attempting to merge this into staging. Please fix manually.

@stage-branch-merger

Copy link
Copy Markdown

I see you added the "On Staging" label, I'll get this merged to the staging branch!

@stage-branch-merger

Copy link
Copy Markdown

Merge conflict attempting to merge this into staging. Please fix manually.

@stage-branch-merger

Copy link
Copy Markdown

I see you added the "On Staging" label, I'll get this merged to the staging branch!

@andrewroth andrewroth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's all for this one -- other changes in here have already been suggested in the other PRs

Comment thread app/models/resource_score.rb
Comment thread app/controllers/resource_default_orders_controller.rb Outdated
Comment thread app/controllers/resource_scores_controller.rb Outdated
Comment thread app/models/resource_score.rb
@stage-branch-merger

Copy link
Copy Markdown

Merge conflict attempting to merge this into staging. Please fix manually.

@stage-branch-merger

Copy link
Copy Markdown

Merge conflict attempting to merge this into staging. Please fix manually.

@JasonBennett41

JasonBennett41 commented Jul 15, 2026

Copy link
Copy Markdown
Author

Hello @andrewroth I have reviewed the 3 PRs (#1910, #1915, #1940) and believe I have rectified the issues highlighted in your comments. The relevant commit hash for those changes is 04e6967.

Since this PR (#1940) contains the latest combined implementation, I believe we should be able to merge it and close the other two.

Regarding the migration safety checks: my understanding is that there are currently no ResourceScore or ResourceDefaultOrder records in the production database, so those concerns may not apply.

You may notice the failed bundle audit check which is unrelated to the work contained here and seems to just be flagging some dependency versions in the Gemfile. I did not take the liberty of upgrading those gems, but let me know if I need to go ahead and do so.

Feel free to reach out with any questions or if some additional changes are needed here! Thank you

@aaronlaib

Copy link
Copy Markdown

Hello @andrewroth I have reviewed the 3 PRs (#1910, #1915, #1940) and believe I have rectified the issues highlighted in your comments. The relevant commit hash for those changes is 04e6967.

Since this PR (#1940) contains the latest combined implementation, I believe we should be able to merge it and close the other two.

Regarding the migration safety checks: my understanding is that there are currently no ResourceScore or ResourceDefaultOrder records in the production database, so those concerns may not apply.

You may notice the failed bundle audit check which is unrelated to the work contained here and seems to just be flagging some dependency versions in the Gemfile. I did not take the liberty of upgrading those gems, but let me know if I need to go ahead and do so.

Feel free to reach out with any questions or if some additional changes are needed here! Thank you

@andrewroth do you know if you have capacity to look at @JasonBennett41's work this week and get this merged? Thanks!

@andrewroth

Copy link
Copy Markdown
Collaborator

Thanks @JasonBennett41 — I went back through my comments on all three PRs
against 04e6967. Nearly everything is addressed, and the InvalidRequestError

Taking your word on the empty production tables — that closes my questions about
MAX_FEATURED_ORDER_POSITION going 10 → 9, the new !featured && featured_order
validation, and the unique index backfill. No further action there.

Three small things before this goes in:

  1. resource_default_orders_controller.rb:127,130 uses
    ResourceScore::MAX_FEATURED_ORDER_POSITION. Should be
    ResourceDefaultOrder::MAX_DEFAULT_ORDER_POSITION — the constant exists and
    the model's own position validation already uses it. Same value today, so
    no behaviour change, but the controller guard and the validation it's
    protecting are currently keyed off two unrelated models. Also copied into
    spec/acceptance/resource_default_orders_controller_spec.rb:388,397.

  2. content_status_controller.rb:34 still has a bare rescue => e. I never
    flagged this file specifically so it's fair that it got missed, but it's the
    same issue as my comment on Added CRUD endpoints for ResourceScores and ResourceDefaultOrders #1915 — and that action takes no user input at
    all, so anything it catches is a bug that should be reaching Rollbar rather
    than turning into a 422.

  3. resources_controller.rb:136 is the one lang lookup that didn't get
    converted: Language.find_by(code: lang_code.downcase). It downcases the
    param but compares against the raw column, so a code like zh-Hans won't
    match. It also falls through to an unfiltered scope when the language isn't
    found, unlike all_default_order_resources right below it.

@stage-branch-merger

Copy link
Copy Markdown

Merge conflict attempting to merge this into staging. Please fix manually.

@JasonBennett41

Copy link
Copy Markdown
Author

@andrewroth Okay sweet. Thank you for your help catching these issues, can't believe I missed the incorrect constants for the default orders. My latest commit should resolve these remaining items, I'm going to merge master in now and make sure the github checks pass 👍🏽

@stage-branch-merger

Copy link
Copy Markdown

Merge conflict attempting to merge this into staging. Please fix manually.

@stage-branch-merger

Copy link
Copy Markdown

Merge conflict attempting to merge this into staging. Please fix manually.

@JasonBennett41

Copy link
Copy Markdown
Author

@andrewroth Okay, all checks passing 👍🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants