Skip to content

Conversation

aaronskiba
Copy link
Contributor

@aaronskiba aaronskiba commented Apr 29, 2025

Changes proposed in this PR:

  • Refactor AnswerController#create_or_update
    • The majority of this refactor extracts logic from the create_or_update action into separate private helper methods (commits 609d5df, efecb33, and a840abb)
    • Commit 3534ee0 maintains the functional behaviour of the destruction of answers, but uses .destroy_all rather than iterating through each individual answer.
  • b40a04c removes an unused variable from app/javascript/src/utils/sectionUpdate.js
  • b025dfe addresses conflicting comments regarding the available locals variables available within app/views/answers/_new_edit.html.erb.

- This refactor addresses the Style/GuardClause offense. It also makes the code easier to read by replacing the large `if @answer.present?` block with the early `return unless @answer.present?`
- The code comment has also been updated to correspond with the refactor.
Copy link

github-actions bot commented Apr 29, 2025

1 Warning
⚠️ There are code changes, but no corresponding tests. Please include tests if this PR introduces any modifications in behavior. \n
Ignore this warning if the PR ONLY contains translation.io synced updates.

Generated by 🚫 Danger

- This refactor simply applies DRY principles to some of the plan fetching code in `AnswersController#create_or_update`.
- Cut and pasted existing answer transaction logic to new `handle_answer_transaction` helper method.
- This refactor is intended to isolate the complex logic for the `Answer.transaction` block and to make the underlying `create_or_update` action easier to read and maintain.
These changes refactor the `Answer.transaction do` block into separate `create_answer`, `update_answer`, and `handle_stale_answer_error` methods.

The changes are almost entirely a direct cut/paste from the original block into the corresponding methods. The only alteration is the prior `if q.question_format.rda_metadata?` check to `return unless q.question_format.rda_metadata?`. This early return check was auto-applied by rubocop.
- This change addresses the following rubocop offences:

```
app/controllers/answers_controller.rb:125:43: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
  def handle_answer_transaction(p_params, q)
                                          ^
app/controllers/answers_controller.rb:144:27: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
  def create_answer(args, q, standards)
                          ^
app/controllers/answers_controller.rb:156:27: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
  def update_answer(args, q, standards)
```
-This change maintains the functional logic of the deletion of answers (based on questions to be removed) while also optimising its efficiency.
- This change updates/corrects the commented locals in `app/views/answers/_new_edit.html.erb`.
- Prior to this change, there were conflicting comments listing the available locals. `plan` does not appear to be one of them. Also, `app/controllers/answers_controller.rb` reveals that `base_template_org` is one of them.
@aaronskiba aaronskiba force-pushed the aaron/conditional-question-fix-for-removing-questions-bug-rails7 branch from 436df3a to b025dfe Compare April 29, 2025 16:36
@aaronskiba aaronskiba marked this pull request as ready for review April 29, 2025 17:28
@aaronskiba aaronskiba changed the title Aaron/conditional question fix for removing questions bug rails7 Refactor AnswersController#create_or_update Apr 29, 2025
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.

1 participant