Mk/review/as/dev/installation completed event listener#5
Merged
printminion-co merged 23 commits intoas/dev/installation_completed_event_listenerfrom Jan 22, 2026
Conversation
…ner and related stubs refactor: extract magic strings to class constants Replace hardcoded magic strings with class constants for better maintainability: - PostSetupJob: JOB_STATUS_DONE, JOB_STATUS_UNKNOWN, JOB_STATUS_CONFIG_KEY - InstallationCompletedEventListener: JOB_STATUS_INIT, JOB_STATUS_CONFIG_KEY, ADMIN_CONFIG_PATH, ADMIN_USER_KEY This improves code readability and reduces the risk of typos when using these values throughout the codebase. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ner and related stubs refactor: use InstallationCompletedEvent properties instead of file parsing Remove initAdminUser() method and config file reading logic in favor of using the event's built-in getAdminUsername() method. This eliminates the need for: - Reading and parsing /vault/secrets/adminconfig - Quote stripping logic - ADMIN_CONFIG_PATH and ADMIN_USER_KEY constants Updated stub to match the real implementation from nextcloud/server#57522 including: - Constructor with dataDirectory, adminUsername, and adminEmail parameters - Getter methods: getDataDirectory(), getAdminUsername(), getAdminEmail() - hasAdminUser() helper method Benefits: - Cleaner code with no file I/O operations - No error handling needed for missing/malformed config files - Uses official Nextcloud API instead of custom parsing - Better type safety with proper event typing Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality refactor: simplify isUrlAvailable method Extract status code to a variable to avoid duplicate method calls and improve readability. Changed from: return response.getStatusCode() >= 200 && response.getStatusCode() < 300; To: statusCode = response.getStatusCode(); return statusCode >= 200 && statusCode < 300; Benefits: - Avoids redundant method call - Slightly better performance - Clearer intent with named variable Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality refactor: remove redundant null check and improve control flow Changed sendInitialWelcomeMail to use early return pattern when user doesn't exist, removing the redundant null check after userExists(). The userManager.get() can still return null in edge cases (e.g., user deleted between exists check and get), so we keep that null check but simplify the control flow. Benefits: - Clearer intent with early returns - Job will now retry if user doesn't exist (doesn't mark as DONE) - Removes unnecessary else block - More consistent error handling Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality refactor: inject ITimeFactory via DI instead of direct instantiation Replace direct instantiation of TimeFactory with dependency injection using ITimeFactory interface in WelcomeMailHelper. Changed from: new TimeFactory() To: Constructor parameter: private ITimeFactory timeFactory Usage: this->timeFactory Benefits: - Better testability with mock injection - Follows dependency injection pattern consistently - Uses interface instead of concrete implementation - Aligns with Nextcloud DI container best practices Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality fix: add validation for overwrite.cli.url configuration Add check to ensure overwrite.cli.url is configured before attempting to use it. If the URL is not set or empty, the job will log a warning and return early, allowing the job to retry on the next cron run in case the configuration is added later. This prevents potential issues with empty URLs being passed to the HTTP client. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality refactor: improve logging with structured context in isUrlAvailable Replace string concatenation with structured logging arrays for better observability: - Use context arrays instead of string concatenation - Change exception logging from debug to info level (more appropriate for network errors) - Improve log message wording: 'Checking URL availability' instead of 'Check URL:' - Add structured context: url and exception message Benefits: - Better log aggregation and filtering in production - Proper log level for transient network issues - Easier to parse and analyze logs Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality refactor: improve log messages with context and appropriate levels Enhanced logging throughout PostSetupJob with structured context and proper log levels: Log level improvements: - Changed job completion to info (was debug) - important milestone - Changed job status unknown to warning (was debug) - indicates potential issue - Changed URL unavailability to info (was debug) - transient expected condition Message improvements: - 'Post-installation job' instead of 'Post install job' for consistency - 'Admin user not found, cannot send welcome email' instead of 'Could not find install user, skip sending welcome mail' - 'System not ready, will retry' instead of 'domain is not ready yet, retry with cron until...' - 'System URL not configured' instead of 'overwrite.cli.url is not configured' Added structured context: - adminUserId in all relevant log messages - config_key for configuration errors - url for availability checks Benefits: - Better log filtering and analysis in production - Clearer user-facing messages - Consistent terminology (email vs mail, system vs domain) - Proper log levels for different scenarios Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality fix: handle null user retrieval and prevent marking job as done Fix edge case where userManager.get() returns null after userExists() check. Previously, if the user was deleted between userExists() and get() calls, the job would still mark itself as DONE without sending the email. Now the job will: - Log an error message with context - Return early without marking as DONE - Retry on next cron run This ensures the welcome email is eventually sent if the user becomes available again. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ner and related stubs Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ner and related stubs Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Add EditorConfig file to maintain consistent coding styles across different editors and IDEs. Configures indent styles for PHP (tabs), JSON/YAML (spaces), and other file types. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Add psalm-baseline.xml to track suppressed static analysis issues. The baseline is currently empty as all issues are properly annotated with @psalm-suppress directives. Existing suppressions are kept because they are legitimate: - PossiblyUnusedMethod: Constructors and methods called by DI container - UndefinedClass: Internal Nextcloud classes not in stubs - MixedAssignment/MixedMethodCall: Unavoidable when using internal classes Baseline file enables tracking if new issues are introduced while maintaining existing suppressions. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…onfig Add stub for OCA\Settings\Mailer\NewUserMailHelper based on Nextcloud server stable31 version. This eliminates UndefinedClass, MixedAssignment, and MixedMethodCall errors, improving type inference to 100%. Move all psalm suppressions from inline @psalm-suppress annotations to centralized issueHandlers in psalm.xml, following Nextcloud best practices as demonstrated in the mail app. Benefits: - 100% type inference (up from 99.3151%) - Cleaner code without annotation clutter - Centralized configuration easier to maintain - Consistent with Nextcloud ecosystem standards Stub source: https://github.com/nextcloud/server/blob/stable31/apps/settings/lib/Mailer/NewUserMailHelper.php Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Enhanced WelcomeMailHelper tests to properly verify NewUserMailHelper interaction and behavior: - Verify password reset token generation and storage when requested - Verify no token generation when not requested - Verify email sending through NewUserMailHelper.sendMail() - Verify no email sent to users without email addresses - Added proper expectations for l10nFactory.getUserLanguage() - Added proper expectations for timeFactory.getTime() and config.setUserValue() Test coverage increased from 3 tests with 7 assertions to 4 tests with 11 assertions. All tests verify actual NewUserMailHelper behavior through the stub, not just execution without exceptions. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Consolidated test setup and removed redundant mock configurations: - Moved all mock setup into setUp() for better organization - Simplified mock return values (e.g., willReturnArgument(0) for translations) - Removed duplicate mock configurations across tests - Kept only essential assertions that verify actual behavior - Reduced code duplication while maintaining test coverage Tests still verify: - Password reset token generation when requested - No token generation when not requested - Email sending through NewUserMailHelper - No email sent to users without email addresses All 17 tests passing with 47 assertions. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
printminion-co
merged commit 57b7346
into
as/dev/installation_completed_event_listener
17 of 28 checks passed
printminion-co
deleted the
mk/review/as/dev/installation_completed_event_listener
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.