Fixes after review

lib/Db/Option.php

@@ -115,17 +115,21 @@ public function __construct() {
 	}
 
 	/**
-	 * Clone this option and reset the id, pollId, deleted
-	 * and confirmed status, as well as the owner and the hash.
-	 *
-	 * @return void
+	 * Build a fresh Option entity pre-populated from this option's data,
+	 * assigned to $toPollId. The fresh entity's id is never set, so
+	 * QBMapper::insert() relies on the DB autoincrement to assign a fresh id.
 	 */
-	public function __clone() {
-		$this->setDeleted(0);
-		$this->setConfirmed(0);
-		$this->setOwner('');
-		/** @psalm-suppress UndefinedMagicMethod */
-		$this->setPollId(0);
+	public function createClone(int $toPollId): self {
+		$clone = new self();
+		$clone->setPoll($toPollId);
+		if ($this->getTimestamp() !== 0) {
+			$clone->setDateTime($this->getDateTime());
+			$clone->setInterval($this->getInterval());
+		} else {
+			$clone->setText($this->getPollOptionText());
+			$clone->setOrder($this->order);
+		}
+		return $clone;
 	}
 
 	/**
@@ -200,6 +204,10 @@ public function getPollOptionHash(): string {
 			$this->getPollOptionText(),
 		);
 	}
+
+	public function getPollOptionHashInDB(): string {
+		return $this->pollOptionHash;
+	}
 	/**
 	 * Get the order of the option. If the option has a valid timestamp,
 	 * the order will be determined by the timestamp to ensure correct
@@ -470,6 +478,16 @@ public function getIsLockedByVotesLimit(): bool {
 		return $this->getVoteLimit() && $this->getUserCountYesVotes() >= $this->getVoteLimit();
 	}
 
+	public function getNewVote(string $userId): Vote {
+		$vote = new Vote();
+		$vote->setPollId($this->getPollId());
+		$vote->setUserId($userId);
+		$vote->setVoteOptionText($this->getPollOptionText());
+		$vote->setVoteOptionId($this->getId());
+		$vote->setVoteOptionHash($this->getPollOptionHash());
+
+		return $vote;
+	}
 	/**
 	 * Set option's text, date and time, and duration from a SimpleOption instance.
 	 * This will also sync the option to update the order and hash.

lib/Db/V6/TableManager.php

@@ -635,12 +635,20 @@ private function updateVoteHashes(): array {
 		foreach ($this->voteMapper->getAll(includeNull: true) as $vote) {
 			try {
 				// if the hash of the vote differs from calculated hash update the vote hash
-				if ($vote->getVoteOptionHash() !== Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText())) {
+				if ($vote->getVoteOptionHashInDB() !== Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText())) {
+					$messages[] = 'update voteId ' . $vote->getId()
+						. ' hash:  ' . $vote->getVoteOptionHashInDB()
+						. ':' . Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText())
+						. '\'' . $vote->getVoteOptionText() . '\'';
+
 					$vote->setVoteOptionHash(Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText()));
 					$vote = $this->voteMapper->update($vote);
+
+
 					$updated++;
 				}
 
+
 				$count++;
 
 			} catch (Exception $e) {
@@ -690,9 +698,17 @@ private function updateOptionHashes(): array {
 		foreach ($this->optionMapper->getAll(includeNull: true) as $option) {
 			try {
 				// if the option's hash differs from $actualHash update the option
-				if ($option->getPollOptionHash() !== Hash::getOptionHash($option->getPollId(), $option->getPollOptionText())) {
+				if ($option->getPollOptionHashInDB() !== Hash::getOptionHash($option->getPollId(), $option->getPollOptionText())) {
+
+					$messages[] = 'update optionId ' . $option->getId()
+						. ' hash:  ' . $option->getPollOptionHashInDB()
+						. ':' . Hash::getOptionHash($option->getPollId(), $option->getPollOptionText())
+						. '\'' . $option->getPollOptionText() . '\'';
+
 					$option->setText($option->getPollOptionText());
 					$option = $this->optionMapper->update($option);
+
+
 					$updated++;
 				}
 

lib/Db/Vote.php

@@ -9,6 +9,7 @@
 namespace OCA\Polls\Db;
 
 use JsonSerializable;
+use OCA\Polls\Helper\Hash;
 
 /**
  * @psalm-suppress UnusedProperty
@@ -72,6 +73,34 @@ private function getAnswerSymbol(): string {
 		}
 	}
 
+	/**
+	 * Get the hash for the vote option. This is generated from the option text
+	 * and poll id.
+	 *
+	 * @return string
+	 *
+	 * @psalm-suppress PossiblyUnusedMethod
+	 */
+	public function getVoteOptionHash(): string {
+		return Hash::getOptionHash(
+			$this->getPollId(),
+			$this->getVoteOptionText(),
+		);
+	}
+
+	/**
+	 * Get the hash for the vote option as stored in the database. This is used
+	 * to identify incorrect hashes stored in the db, because getVoteOptionHash()
+	 * which is generated from the option text and poll id.
+	 *
+	 * @return string
+	 *
+	 * @psalm-suppress PossiblyUnusedMethod
+	 */
+	public function getVoteOptionHashInDb(): string {
+		return $this->voteOptionHash;
+	}
+
 	/**
 	 * @return array
 	 *

lib/Model/DateInterval.php

@@ -43,22 +43,56 @@ public function isZeroDuration(): bool {
 	 * Get the ISO 8601 duration string representation of this DateInterval.
 	 * If the interval is zero (all components are zero), this method returns null.
 	 *
+	 * NOTE: Currently not used
+	 *
 	 * @return string|null The ISO 8601 duration string or null if the interval is zero.
+	 *
+	 * @psalm-suppress PossiblyUnusedMethod
 	 */
-	public function getISO(): ?string {
-		if (
-			$this->y === 0
-			&& $this->m === 0
-			&& $this->d === 0
-			&& $this->h === 0
-			&& $this->i === 0
-			&& $this->s === 0
-		) {
+	public function getISOExpanded(): ?string {
+		if ($this->isZeroDuration()) {
 			return null;
 		}
 		return $this->format('P%yY%mM%dDT%hH%iM%sS');
 	}
 
+	/**
+	 * Get the ISO 8601 duration string representation of this DateInterval, rescaled to a more compact form.
+	 * For example, if the interval represents 2 days and 3 hours, it will return "P2DT3H" instead of "P0Y0M2DT3H0M0S".
+	 * If the interval is zero (all components are zero), this method returns null.
+	 *
+	 * @return string The rescaled ISO 8601 duration string.
+	 */
+	public function getISO(): ?string {
+		if ($this->isZeroDuration()) {
+			return null;
+		}
+
+		$parts = ['P'];
+		if ($this->y > 0) {
+			$parts[] = $this->y . 'Y';
+		}
+		if ($this->m > 0) {
+			$parts[] = $this->m . 'M';
+		}
+		if ($this->d > 0) {
+			$parts[] = $this->d . 'D';
+		}
+		if ($this->h > 0 || $this->i > 0 || $this->s > 0) {
+			$parts[] = 'T';
+			if ($this->h > 0) {
+				$parts[] = $this->h . 'H';
+			}
+			if ($this->i > 0) {
+				$parts[] = $this->i . 'M';
+			}
+			if ($this->s > 0) {
+				$parts[] = $this->s . 'S';
+			}
+		}
+		return implode('', $parts);
+	}
+
 	/**
 	 * Determine if this DateInterval represents one or more full days.
 	 *

lib/Service/MailService.php

@@ -178,26 +178,20 @@ public function sendNotifications(): void {
 		}
 	}
 
-	public function sendInvitation(Share $share, ?SentResult &$sentResult = null): ?SentResult {
+	public function sendInvitation(Share $share, SentResult &$sentResult = new SentResult()): SentResult {
 		foreach ($this->userMapper->getUserFromShare($share)->getMembers() as $recipient) {
 			$invitation = new InvitationMail($recipient->getId(), $share);
 
 			try {
 				$invitation->send();
-				if ($sentResult) {
-					$sentResult->AddSentMail($recipient);
-				}
+				$sentResult->AddSentMail($recipient);
 
 			} catch (InvalidEmailAddress $e) {
-				if ($sentResult) {
-					$sentResult->AddAbortedMail($recipient, SentResult::INVALID_EMAIL_ADDRESS);
-				}
+				$sentResult->AddAbortedMail($recipient, SentResult::INVALID_EMAIL_ADDRESS);
 				$this->logger->warning('Invalid or no email address for invitation', ['recipient' => json_encode($recipient)]);
 
 			} catch (\Exception $e) {
-				if ($sentResult) {
-					$sentResult->AddAbortedMail($recipient);
-				}
+				$sentResult->AddAbortedMail($recipient);
 
 				$this->logger->error('Error sending invitation', [
 					'recipient' => json_encode($recipient),

lib/Service/OptionService.php

@@ -373,8 +373,7 @@ public function clone(int $fromPollId, int $toPollId): void {
 			->request(Poll::PERMISSION_OPTION_ADD);
 
 		foreach ($this->optionMapper->findByPoll($fromPollId) as $origin) {
-			$option = clone $origin;
-			$option->setPoll($toPollId);
+			$option = $origin->createClone($toPollId);
 			$option = $this->optionMapper->insert($option);
 			$this->eventDispatcher->dispatchTyped(new OptionCreatedEvent($option));
 		}

lib/Service/ShareService.php

@@ -496,11 +496,11 @@ private function lock(Share $share, bool $unlock = false): Share {
 		return $share;
 	}
 
-	public function sendAllInvitations(int $pollId): ?SentResult {
+	public function sendAllInvitations(int $pollId): SentResult {
 		$sentResult = new SentResult();
+		$shares = $this->listNotInvited($pollId);
 
 		// first resolve group shares
-		$shares = $this->listNotInvited($pollId);
 		foreach ($shares as $share) {
 			if (in_array($share->getType(), Share::RESOLVABLE_SHARES)) {
 				$this->resolveGroup($share);
@@ -560,7 +560,7 @@ private function resolveGroup(Share $share): array {
 	 * @param Share $share
 	 * @param SentResult $sentResult to collect results
 	 */
-	public function sendInvitation(Share $share, ?SentResult &$sentResult = null): ?SentResult {
+	public function sendInvitation(Share $share, SentResult &$sentResult = new SentResult()): SentResult {
 		if (in_array($share->getType(), [Share::TYPE_USER, Share::TYPE_ADMIN], true)) {
 			$this->notificationService->sendInvitation($share->getPollId(), $share->getUserId());
 		} elseif ($share->getType() === Share::TYPE_GROUP) {

lib/Service/SubscriptionService.php

@@ -15,7 +15,6 @@
 use OCA\Polls\Exceptions\ForbiddenException;
 use OCA\Polls\UserSession;
 use OCP\AppFramework\Db\DoesNotExistException;
-use OCP\DB\Exception;
 
 class SubscriptionService {
 	/** @psalm-suppress PossiblyUnusedMethod */
@@ -44,7 +43,13 @@ public function set(bool $setToSubscribed, int $pollId): bool {
 		try {
 			if ($setToSubscribed) {
 				$this->pollMapper->get($pollId)->request(Poll::PERMISSION_POLL_SUBSCRIBE);
-				$this->add($pollId, $this->userSession->getCurrentUserId());
+				try {
+					$this->subscriptionMapper->findByPollAndUser($pollId, $this->userSession->getCurrentUserId());
+					// Already subscribed — nothing to do
+					return true;
+				} catch (DoesNotExistException) {
+					$this->add($pollId, $this->userSession->getCurrentUserId());
+				}
 			} else {
 				$subscription = $this->subscriptionMapper->findByPollAndUser($pollId, $this->userSession->getCurrentUserId());
 				$this->subscriptionMapper->delete($subscription);
@@ -57,12 +62,6 @@ public function set(bool $setToSubscribed, int $pollId): bool {
 		} catch (ForbiddenException $e) {
 			// Is not allowed to subscribe
 			return false;
-		} catch (Exception $e) {
-			if ($e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
-				// Is already subscribed
-				return true;
-			}
-			throw $e;
 		}
 	}
 

lib/Service/VoteService.php

@@ -54,18 +54,6 @@ public function list(int $pollId): array {
 		return $votes;
 	}
 
-	private function checkLimits(Option $option): void {
-		// check, if the optionlimit is reached or exceeded, if one is set
-		if ($option->getIsLockedByOptionLimit()) {
-			throw new VoteLimitExceededException;
-		}
-
-		if ($option->getIsLockedByVotesLimit()) {
-			throw new VoteLimitExceededException;
-		}
-		return;
-	}
-
 	private function checkVoteLimit(Option $option): void {
 		// check, if the optionlimit is reached or exceeded, if one is set
 		if ($option->getIsLockedByOptionLimit()) {
@@ -75,17 +63,16 @@ private function checkVoteLimit(Option $option): void {
 		if ($option->getIsLockedByVotesLimit()) {
 			throw new VoteLimitExceededException;
 		}
-		return;
 	}
 
 	/**
 	 * Set vote
 	 */
-	public function set(Option|int $optionOrOptionIdoptionId, string $setTo): ?Vote {
-		if ($optionOrOptionIdoptionId instanceof Option) {
-			$option = $optionOrOptionIdoptionId;
+	public function set(Option|int $optionOrOptionId, string $setTo): ?Vote {
+		if ($optionOrOptionId instanceof Option) {
+			$option = $optionOrOptionId;
 		} else {
-			$option = $this->optionMapper->find($optionOrOptionIdoptionId);
+			$option = $this->optionMapper->find($optionOrOptionId);
 		}
 		$poll = $this->pollMapper->get($option->getPollId())
 			->request(Poll::PERMISSION_VOTE_EDIT);
@@ -103,27 +90,24 @@ public function set(Option|int $optionOrOptionIdoptionId, string $setTo): ?Vote
 			}
 
 			if ($setTo === Vote::VOTE_YES) {
-				$this->checkLimits($option);
+				$this->checkVoteLimit($option);
 			}
 
 			//  delete no votes, if poll setting is set to useNo === 0
 			$deleteVoteInsteadOfNoVote = in_array(trim($setTo), [Vote::VOTE_NO, '']) && !boolval($poll->getUseNo());
 
 			if ($deleteVoteInsteadOfNoVote) {
-				$this->vote->setVoteAnswer('');
 				$this->voteMapper->delete($this->vote);
+				$this->eventDispatcher->dispatchTyped(new VoteSetEvent($this->vote));
+				return null;
 			} else {
 				$this->vote->setVoteAnswer($setTo);
 				$this->vote = $this->voteMapper->update($this->vote);
 			}
 		} catch (DoesNotExistException $e) {
 			// Vote does not exist, insert as new Vote
-			$this->vote = new Vote();
+			$this->vote = $option->getNewVote($this->userSession->getCurrentUserId());
 
-			$this->vote->setPollId($poll->getId());
-			$this->vote->setUserId($this->userSession->getCurrentUserId());
-			$this->vote->setVoteOptionText($option->getPollOptionText());
-			$this->vote->setVoteOptionId($option->getId());
 			$this->vote->setVoteAnswer($setTo);
 			$this->vote = $this->voteMapper->insert($this->vote);
 		}
@@ -174,6 +158,7 @@ private function delete(int $pollId, string $userId, bool $deleteOnlyOrphaned =
 		// fake a vote so that the event can be triggered
 		// suppress logging of this action
 		$this->vote = new Vote();
+
 		$this->vote->setPollId($pollId);
 		$this->vote->setUserId($userId);
 		$this->voteMapper->deleteByPollAndUserId($pollId, $userId);