diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php index 7a3ed703e..b121c8e78 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php @@ -3684,8 +3684,8 @@ public function updateExtraQuestion($summit_id, $sponsor_id, $extra_question_id) #[OA\Delete( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}", - description: "required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, - summary: 'Delete Sponsor Extra Question', + summary: 'Delete Sponsor Extra Question (always refused)', + description: "Always refused with 412: devices may hold answers for this question that the server has not seen yet. Edit the wording or turn off Mandatory instead. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, operationId: 'deleteSponsorExtraQuestion', tags: ['Sponsors'], x: [ @@ -3730,8 +3730,8 @@ public function updateExtraQuestion($summit_id, $sponsor_id, $extra_question_id) ], responses: [ new OA\Response( - response: Response::HTTP_NO_CONTENT, - description: 'Extra question deleted successfully' + response: Response::HTTP_PRECONDITION_FAILED, + description: "Validation Error - always returned: questions can't be deleted once created, because devices may already be using them" ), new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), @@ -3979,8 +3979,8 @@ function ($value_id, $payload, $summit, $sponsor_id, $extra_question_id) { use ParametrizedDeleteEntity; #[OA\Delete( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/values/{value_id}", - description: "required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, - summary: 'Delete Extra Question Value', + description: "Always refused with 412: devices may hold answers for this option that the server has not seen yet. Edit the wording instead. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, + summary: 'Delete Extra Question Value (always refused)', operationId: 'deleteSponsorExtraQuestionValue', tags: ['Sponsors'], x: [ @@ -4032,8 +4032,8 @@ function ($value_id, $payload, $summit, $sponsor_id, $extra_question_id) { ], responses: [ new OA\Response( - response: Response::HTTP_NO_CONTENT, - description: 'Question value deleted successfully' + response: Response::HTTP_PRECONDITION_FAILED, + description: "Validation Error - always returned: answer options can't be deleted once created, because devices may already be using them" ), new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), diff --git a/app/Services/Model/Imp/SponsorUserInfoGrantService.php b/app/Services/Model/Imp/SponsorUserInfoGrantService.php index 82fc7865e..f4c47bdf5 100644 --- a/app/Services/Model/Imp/SponsorUserInfoGrantService.php +++ b/app/Services/Model/Imp/SponsorUserInfoGrantService.php @@ -408,12 +408,8 @@ private function addBadgeScanLocked( // extra questions $extra_questions = $data['extra_questions'] ?? []; - if (count($extra_questions)) { - $res = $scan->hadCompletedExtraQuestions($extra_questions); - if (!$res) { - throw new ValidationException("You neglected to fill in all mandatory questions for the badge scan."); - } - } + if (count($extra_questions)) + $this->applyExtraQuestionAnswers($scan, $extra_questions, 'addBadgeScan'); return $scan; }); @@ -477,7 +473,6 @@ private function addBadgeScanLocked( * @param SponsorBadgeScan $existing * @param array $data * @return SponsorBadgeScan - * @throws ValidationException */ private function mergeRetryIntoExistingScan(SponsorBadgeScan $existing, array $data): SponsorBadgeScan { @@ -486,12 +481,82 @@ private function mergeRetryIntoExistingScan(SponsorBadgeScan $existing, array $d $existing->setNotes($notes); $extra_questions = $data['extra_questions'] ?? []; - if (count($extra_questions) && !$existing->hadCompletedExtraQuestions($extra_questions)) - throw new ValidationException("You neglected to fill in all mandatory questions for the badge scan."); + if (count($extra_questions)) + $this->applyExtraQuestionAnswers($existing, $extra_questions, 'mergeRetryIntoExistingScan'); return $existing; } + /** + * "Mandatory" and the allowed options are instructions to the device, not + * server rules: a device can hold answers captured under a question setup + * the server no longer has (e.g. a mandatory Text question switched to a + * ComboBox, so a free-text answer matches no option). Save whatever answers + * were sent and never reject the upload or roll the scan back over them. + * + * hadCompletedExtraQuestions adds every answer before validating them and + * nothing is flushed yet, so catching its ValidationException keeps both + * the scan and the answers. + * @param SponsorBadgeScan $scan + * @param array $extra_questions + * @param string $caller + */ + private function applyExtraQuestionAnswers(SponsorBadgeScan $scan, array $extra_questions, string $caller): void + { + try { + if (!$scan->hadCompletedExtraQuestions($extra_questions)) + Log::warning( + sprintf( + "SponsorUserInfoGrantService::%s badge scan was saved with missing mandatory answers - %s", + $caller, + $this->describeScanForLog($scan) + ) + ); + } + catch (ValidationException $ex) { + Log::warning( + sprintf( + "SponsorUserInfoGrantService::%s badge scan was saved with answers that do not match the question setup (%s) - %s", + $caller, + $ex->getMessage(), + $this->describeScanForLog($scan) + ) + ); + } + } + + /** + * Ids support needs to find a scan saved with incomplete answers. A new + * scan has no id until the transaction flushes (logged as 0); sponsor, + * badge, member and scan date identify it. + * @param SponsorBadgeScan $scan + * @return string + */ + private function describeScanForLog(SponsorBadgeScan $scan): string + { + $answered = []; + foreach ($scan->getExtraQuestionAnswers() as $answer) { + if ($answer->hasValue()) + $answered[$answer->getQuestionId()] = true; + } + + $unanswered_mandatory = []; + foreach ($scan->getExtraQuestions() as $question) { + if ($question->isMandatory() && !isset($answered[$question->getId()])) + $unanswered_mandatory[] = $question->getId(); + } + + return sprintf( + "scan %s sponsor %s badge %s member %s scan_date %s unanswered mandatory questions [%s]", + $scan->getId(), + $scan->getSponsor()->getId(), + $scan->getBadge()->getId(), + $scan->getUser()->getId(), + $scan->getScanDate()->getTimestamp(), + implode(",", $unanswered_mandatory) + ); + } + /** * @param Summit $summit * @param Member $current_member @@ -521,12 +586,8 @@ public function updateBadgeScan(Summit $summit, Member $current_member, int $sca // extra questions $extra_questions = $payload['extra_questions'] ?? []; - if (count($extra_questions)) { - $res = $scan->hadCompletedExtraQuestions($extra_questions); - if (!$res) { - throw new ValidationException("You neglected to fill in all mandatory questions for the badge scan."); - } - } + if (count($extra_questions)) + $this->applyExtraQuestionAnswers($scan, $extra_questions, 'updateBadgeScan'); return $scan; }); diff --git a/app/Services/Model/Imp/SummitSponsorService.php b/app/Services/Model/Imp/SummitSponsorService.php index 282e1a146..b464a0279 100644 --- a/app/Services/Model/Imp/SummitSponsorService.php +++ b/app/Services/Model/Imp/SummitSponsorService.php @@ -1084,18 +1084,10 @@ public function updateSponsorExtraQuestion(Summit $summit, int $sponsor_id, int */ public function deleteSponsorExtraQuestion(Summit $summit, int $sponsor_id, int $extra_question_id): void { - $this->tx_service->transaction(function () use ($summit, $sponsor_id, $extra_question_id) { - $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); - if (is_null($summit_sponsor)) - throw new EntityNotFoundException("Sponsor not found."); - - $extra_question = $summit_sponsor->getExtraQuestionById($extra_question_id); - - if(!$extra_question instanceof SummitSponsorExtraQuestionType) - throw new EntityNotFoundException("Sponsor extra question not found."); - - $summit_sponsor->removeExtraQuestion($extra_question); - }); + // Always refused: a device may hold answers for this question that the server has not seen yet. + throw new ValidationException( + "Questions can't be deleted once created, because devices may already be using them. You can edit the wording, turn off Mandatory, or contact support." + ); } /** @@ -1160,19 +1152,10 @@ public function updateExtraQuestionValue(Summit $summit, int $sponsor_id, int $q */ public function deleteExtraQuestionValue(Summit $summit, int $sponsor_id, int $question_id, int $value_id): void { - $this->tx_service->transaction(function () use ($summit, $sponsor_id, $question_id, $value_id) { - $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); - if (is_null($summit_sponsor)) - throw new EntityNotFoundException("Sponsor not found."); - - $extra_question = $summit_sponsor->getExtraQuestionById($question_id); - - if(!$extra_question instanceof SummitSponsorExtraQuestionType) - throw new EntityNotFoundException("Sponsor extra question not found."); - - - parent::_deleteExtraQuestionValue($extra_question, $value_id); - }); + // Always refused: a device may hold answers for this option that the server has not seen yet. + throw new ValidationException( + "Answer options can't be deleted once created, because devices may already be using them. You can edit the wording, or contact support." + ); } /** diff --git a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php index 4f761eafb..f8e1e66dd 100644 --- a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php @@ -12,6 +12,8 @@ * limitations under the License. **/ +use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeConstants; +use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeValue; use App\Models\Foundation\Main\IGroup; use App\Models\Foundation\Summit\ExtraQuestions\SummitSponsorExtraQuestionType; use App\Models\Foundation\Summit\Repositories\ISponsorRepository; @@ -1493,4 +1495,158 @@ public function testAddBadgeScanUniqueViolationAppliesNotesToTheWinningScan(){ $this->assertEquals('hot lead, follow up', $this->reloadScan($winner_id)->getNotes(), "notes of the request that lost the race must be persisted on the winning scan"); } + + /** + * "Mandatory" is an instruction to the device, not a server rule: sets the + * sponsor's first two questions as mandatory and returns their ids. + * @return array + */ + private function makeFirstTwoSponsorQuestionsMandatory(): array + { + $sponsor = self::$summit->getSummitSponsors()[0]; + $ids = []; + foreach ([0, 1] as $idx) { + $question = $sponsor->getExtraQuestions()[$idx]; + if (!$question instanceof SummitSponsorExtraQuestionType) self::fail(); + $question->setMandatory(true); + $ids[] = $question->getId(); + } + self::$em->persist($sponsor); + self::$em->flush(); + return $ids; + } + + /** + * A scan captured offline and answered before reconnecting reaches the + * server as a first POST that carries answers. A mandatory question left + * unanswered must neither reject it nor roll the scan back. + */ + public function testAddBadgeScanWithMissingMandatoryAnswerIsSavedWithItsOtherAnswers(){ + [$answered_id, ] = $this->makeFirstTwoSponsorQuestionsMandatory(); + + $service = App::make(ISponsorUserInfoGrantService::class); + $scan = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'extra_questions' => [ + ['question_id' => $answered_id, 'answer' => 'None'], + ], + ])); + + $answers = $this->reloadScan($scan->getId())->getExtraQuestionAnswers(); + $this->assertCount(1, $answers); + $this->assertEquals($answered_id, $answers->first()->getQuestionId()); + } + + public function testAddBadgeScanRetryWithMissingMandatoryAnswerIsSaved(){ + [$answered_id, ] = $this->makeFirstTwoSponsorQuestionsMandatory(); + + $service = App::make(ISponsorUserInfoGrantService::class); + $first = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload()); + $first_id = $first->getId(); + + $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'extra_questions' => [ + ['question_id' => $answered_id, 'answer' => 'None'], + ], + ])); + + $answers = $this->reloadScan($first_id)->getExtraQuestionAnswers(); + $this->assertCount(1, $answers); + $this->assertEquals($answered_id, $answers->first()->getQuestionId()); + } + + public function testUpdateBadgeScanWithMissingMandatoryAnswerIsSaved(){ + [$answered_id, ] = $this->makeFirstTwoSponsorQuestionsMandatory(); + + $service = App::make(ISponsorUserInfoGrantService::class); + $scan = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload()); + $scan_id = $scan->getId(); + + $service->updateBadgeScan(self::$summit, self::$member, $scan_id, [ + 'extra_questions' => [ + ['question_id' => $answered_id, 'answer' => 'None'], + ], + ]); + + $answers = $this->reloadScan($scan_id)->getExtraQuestionAnswers(); + $this->assertCount(1, $answers); + $this->assertEquals($answered_id, $answers->first()->getQuestionId()); + } + + /** + * The sponsor switches the first question (Text) to a mandatory ComboBox + * while devices still hold free-text answers for it. Returns its id. + * @return int + */ + private function switchFirstSponsorQuestionToMandatoryComboBox(): int + { + $sponsor = self::$summit->getSummitSponsors()[0]; + $question = $sponsor->getExtraQuestions()[0]; + if (!$question instanceof SummitSponsorExtraQuestionType) self::fail(); + $question->setType(ExtraQuestionTypeConstants::ComboBoxQuestionType); + $question->setMandatory(true); + $value = new ExtraQuestionTypeValue(); + $value->setLabel('Yes'); + $value->setValue('Yes'); + $question->addValue($value); + self::$em->persist($sponsor); + self::$em->flush(); + return $question->getId(); + } + + /** + * A free-text answer ("Yes") matches no option id of the ComboBox, which + * makes the question validation throw; it must neither reject the upload + * nor roll the scan back. + */ + public function testAddBadgeScanWithAnswerNotMatchingTheQuestionOptionsIsSaved(){ + $question_id = $this->switchFirstSponsorQuestionToMandatoryComboBox(); + + $service = App::make(ISponsorUserInfoGrantService::class); + $scan = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'extra_questions' => [ + ['question_id' => $question_id, 'answer' => 'Yes'], + ], + ])); + + $answers = $this->reloadScan($scan->getId())->getExtraQuestionAnswers(); + $this->assertCount(1, $answers); + $this->assertEquals($question_id, $answers->first()->getQuestionId()); + $this->assertEquals('Yes', $answers->first()->getValue()); + } + + public function testAddBadgeScanRetryWithAnswerNotMatchingTheQuestionOptionsIsSaved(){ + $question_id = $this->switchFirstSponsorQuestionToMandatoryComboBox(); + + $service = App::make(ISponsorUserInfoGrantService::class); + $first = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload()); + $first_id = $first->getId(); + + $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'extra_questions' => [ + ['question_id' => $question_id, 'answer' => 'Yes'], + ], + ])); + + $answers = $this->reloadScan($first_id)->getExtraQuestionAnswers(); + $this->assertCount(1, $answers); + $this->assertEquals('Yes', $answers->first()->getValue()); + } + + public function testUpdateBadgeScanWithAnswerNotMatchingTheQuestionOptionsIsSaved(){ + $question_id = $this->switchFirstSponsorQuestionToMandatoryComboBox(); + + $service = App::make(ISponsorUserInfoGrantService::class); + $scan = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload()); + $scan_id = $scan->getId(); + + $service->updateBadgeScan(self::$summit, self::$member, $scan_id, [ + 'extra_questions' => [ + ['question_id' => $question_id, 'answer' => 'Yes'], + ], + ]); + + $answers = $this->reloadScan($scan_id)->getExtraQuestionAnswers(); + $this->assertCount(1, $answers); + $this->assertEquals('Yes', $answers->first()->getValue()); + } } diff --git a/tests/oauth2/OAuth2SummitSponsorApiTest.php b/tests/oauth2/OAuth2SummitSponsorApiTest.php index 374c2de7c..6fbf2aa0e 100644 --- a/tests/oauth2/OAuth2SummitSponsorApiTest.php +++ b/tests/oauth2/OAuth2SummitSponsorApiTest.php @@ -522,23 +522,21 @@ public function testAddSponsorUserMember(){ return $sponsor; } + /** + * Sponsors are seeded with the maximum number of extra questions and the API + * no longer deletes them, so make room directly on the model. + * using last() keeps order numbering compact so new question order == count + */ + private function removeLastSponsorExtraQuestion(){ + $sponsor = self::$sponsors[0]; + $sponsor->removeExtraQuestion($sponsor->getExtraQuestions()->last()); + self::$em->persist($sponsor); + self::$em->flush(); + } + public function testAddSponsorExtraQuestions(){ - // remove the last extra question first (sponsors already have 5, the max) - // using last() keeps order numbering compact so new question order == count - $existingQuestion = self::$sponsors[0]->getExtraQuestions()->last(); - $this->action( - "DELETE", - "OAuth2SummitSponsorApiController@deleteExtraQuestion", - [ - 'id' => self::$summit->getId(), - 'sponsor_id' => self::$sponsors[0]->getId(), - 'extra_question_id' => $existingQuestion->getId() - ], - [], [], [], - $this->getAuthHeaders() - ); - $this->assertResponseStatus(204); + $this->removeLastSponsorExtraQuestion(); $params = [ 'id' => self::$summit->getId(), @@ -711,7 +709,9 @@ public function testDeleteSponsorExtraQuestionsBySponsor(){ $this->getAuthHeaders() ); - $this->assertResponseStatus(204); + // questions can never be deleted: devices may hold answers the server has not seen + $this->assertResponseStatus(412); + $this->assertStringContainsString("Questions can't be deleted once created", $response->getContent()); } public function testAddLeadReportSettings(){ @@ -1499,21 +1499,7 @@ public function testDeleteSocialNetwork(){ // ---- Extra Question Values CRUD ---- private function createComboBoxExtraQuestion(){ - // remove the last extra question first (sponsors already have 5, the max) - $existingQuestion = self::$sponsors[0]->getExtraQuestions()->last(); - $this->action( - "DELETE", - "OAuth2SummitSponsorApiController@deleteExtraQuestion", - [ - 'id' => self::$summit->getId(), - 'sponsor_id' => self::$sponsors[0]->getId(), - 'extra_question_id' => $existingQuestion->getId() - ], - [], [], [], - $this->getAuthHeaders() - ); - $this->assertResponseStatus(204); - + $this->removeLastSponsorExtraQuestion(); $params = [ 'id' => self::$summit->getId(), 'sponsor_id' => self::$sponsors[0]->getId(), @@ -1630,7 +1616,9 @@ public function testDeleteExtraQuestionValue(){ $this->getAuthHeaders() ); - $this->assertResponseStatus(204); + // answer options can never be deleted: devices may hold answers the server has not seen + $this->assertResponseStatus(412); + $this->assertStringContainsString("Answer options can't be deleted once created", $response->getContent()); } // ---- Public API ----