Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(survey): [PPT-67|PPT-101] update /surveys/questions endpoint #257

Merged
merged 6 commits into from
Feb 21, 2023

Conversation

chillfox
Copy link
Contributor

@chillfox chillfox commented Feb 16, 2023

Get endpoint for survey questions:

  • Filter on deleted

Update endpoint for survey questions:

  • Update the question if there are NO linked answers.
  • Create a new question if there are linked answers.
  • Soft-delete the question if there are linked answers.

Delete endpoint for survey questions:

  • Delete the question if there are NO linked answers.
  • Soft-delete the question if there are linked answers.

@chillfox chillfox self-assigned this Feb 16, 2023
@github-actions github-actions bot added the type: enhancement new feature or request label Feb 16, 2023
@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Feb 16, 2023
@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Feb 20, 2023
@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Feb 20, 2023
@chillfox chillfox marked this pull request as ready for review February 20, 2023 06:19
@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Feb 20, 2023
@chillfox chillfox requested a review from naqvis February 20, 2023 06:20
Comment on lines +23 to +25
if question_model.persisted? && Survey::Answer.query.where(question_id: question_model.id).count > 0
question_model.soft_delete
question_model.clear_persisted
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having hard time in understanding the logic here. We are updating the field in database, but mutating ivars to represent state which is different from db.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

question_model.persisted? is true if the model is in the database.

Survey::Answer.query.where(question_id: question_model.id).count > 0 is true if there are any linked answers.

So if the model is in the database and there are linked answers, then:

  • question_model.soft_delete we mark it as deleted
  • question_model.clear_persisted clears the persistent state so that our changed model gets inserted as a new row in the database and gets a new id.

By doing this we ensure that questions with linked answers are never changed as changing the question could change the meaning of the answers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, Clear does not have the ability to cancel update/insert in the lifecycle hooks, so you either have to work around that or move the logic to the controllers (putting it in the controllers, feels like a future bug waiting to happen).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

question_model.soft_delete uses Clear::SQL.execute Clear::SQL.raw(...) to bypass the lifecycle hooks. If it did not bypass the lifecycle hooks then it would create an infinite loop.

@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Feb 21, 2023
Copy link
Contributor

@naqvis naqvis left a comment

Choose a reason for hiding this comment

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

LGTM

@chillfox chillfox merged commit c0af0ff into master Feb 21, 2023
@chillfox chillfox deleted the PPT-67-ability-to-edit-question-in-question-bank branch February 21, 2023 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants