Open
Conversation
Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
…rvice Add GithubSyncService for syncing GitHub documents to database
Introduce ICrystaDocumentService for batch document persistence, with EF Core and in-memory implementations for production and testing. Refactor GithubSyncService to use this abstraction, improving separation of concerns and testability. Update DI registrations and rename fake program repository for consistency. Sync module service now loads related CrystaProgram entities. Note: tasks.json changes include duplicate build/test tasks.
For each document with a valid SourceHtmlUrl, fetch the file content asynchronously from GitHub if not already loaded, then process it with GetHtmlContent(). This ensures documents are properly loaded and processed before being added to the result list. Enhance document content retrieval from GitHub Fetch and process document content from GitHub when SourceHtmlUrl is present, ensuring documents have up-to-date content before being added to the result list.
- Comment out unique index on CrystaDocument.Code to allow duplicates - Remove null chars from document content after HTML generation - Use SHA hash of HtmlUrl for SyncId instead of raw URL - Update processed key logic in GithubSyncService to use SyncId - Add missing using for extension methods in GitHubExtensions
Implemented GetDocumentsByCrystaUrlAsync and GetDocumentsByProgramCodeAsync in ICrystaDocumentService and its main implementation. Updated DocumentController to use these methods for fetching documents and selecting language variants. Changed SyncInfoDto.SyncId from Guid? to string? for greater flexibility.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements GitHub document synchronization functionality for the CrystaLearn application. The changes introduce a new sync service to fetch and synchronize documents from GitHub repositories into the local database, expanding the existing sync infrastructure beyond Azure Board.
Key Changes
- Changed
SyncInfoDto.SyncIdfromGuid?tostring?to accommodate GitHub's string-based identifiers - Implemented
GithubSyncServiceto handle GitHub document synchronization with change detection and incremental updates - Added
CrystaDocumentServiceand related interfaces for managing document persistence and retrieval
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| SyncInfoDto.cs | Changed SyncId type from Guid to string for GitHub compatibility |
| DocumentController.cs | Refactored to use new CrystaDocumentService and improved culture-based document selection logic |
| ICrystaDocumentService.cs | New interface defining document service contract |
| GithubSyncService.cs | New service implementing GitHub document sync with hash-based change detection |
| CrystaDocumentServiceFake.cs | Test fake implementation for document service |
| CrystaDocumentService.cs | Production implementation for document CRUD operations |
| CrystaProgramSyncService.cs | Added GitHub sync module type handling |
| DocumentRepositoryInMemory.cs | Updated type references from Document to CrystaDocument |
| DocumentRepositoryDirectGitHub.cs | Enhanced with content fetching logic |
| CrystaDocument.cs | Added Table attribute and DisplayCode property |
| ApplicationBuilderExtensions.cs | Updated DI configuration for new services |
| CrystaDocumentConfiguration.cs | New EF Core entity configuration |
| AppDbContext.cs | Added CrystaDocument DbSet |
| GithubSyncServiceTests.cs | New test coverage for GitHub sync functionality |
| .vscode/tasks.json | Configuration changes (contains duplicate tasks) |
| Migration files | Database schema updates for CrystaDocument table |
Files not reviewed (2)
- src/Core/CrystaLearn.Core/Migrations/20251220121952_AddCrystaDocument.Designer.cs: Language not supported
- src/Core/CrystaLearn.Core/Migrations/20251226123528_AddDisplayCodeToCrytaDocument.Designer.cs: Language not supported
src/Core/CrystaLearn.Core/Services/CrystaProgramSyncModuleService.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Refactor all IGithubSyncService/GithubSyncService references to IGitHubSyncService/GitHubSyncService for consistent naming. Implement GetDocumentByCrystaUrlAsync in DocumentRepositoryDirectGitHub to support culture-aware document retrieval and content fetching from GitHub. Update service registration, dependency injection, and test class names accordingly. Improve null handling and code clarity in the new method.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 29 changed files in this pull request and generated 4 comments.
Files not reviewed (2)
- src/Core/CrystaLearn.Core/Migrations/20251220121952_AddCrystaDocument.Designer.cs: Language not supported
- src/Core/CrystaLearn.Core/Migrations/20251226123528_AddDisplayCodeToCrytaDocument.Designer.cs: Language not supported
| subgraph "CrystaLearn Sync Layer" | ||
| ABS[AzureBoardSyncService] | ||
| GHS[GitHubService] | ||
| GHS[GithubSyncService] |
There was a problem hiding this comment.
Inconsistent naming: the service is named "GithubSyncService" but the documentation refers to it as "GithubSyncService" (lowercase 'h'). The standard casing should be "GitHub" with a capital 'H'. This inconsistency appears throughout the codebase.
Suggested change
| GHS[GithubSyncService] | |
| GHS[GitHubSyncService] |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
|
@afshinalizadeh I've opened a new pull request, #114, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
Add null safety for SyncId in GithubSyncService
Contributor
|
@afshinalizadeh I've opened a new pull request, #115, to work on those changes. Once the pull request is ready, I'll request review from you. |
- Removed static keyword from _modules field to prevent shared state issues - Changed from AppDbContext to IDbContextFactory<AppDbContext> for proper singleton support - Added lazy initialization with thread-safe semaphore lock - Changed service registration from Transient to Singleton - Updated Fake service with same pattern for consistency Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
- Add Microsoft.EntityFrameworkCore using statement for EF Core extensions - Add thread-safe synchronization for UpdateSyncModuleAsync with SemaphoreSlim - Move initialization to async method with proper thread-safety in Fake service - Add synchronization locks for in-memory list modifications Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
- Add CancellationToken parameter to interface method with default value - Propagate cancellation token through all database operations - Pass cancellation token to AddAsync and SaveChangesAsync methods Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
- Add IDisposable implementation to both service classes - Properly dispose SemaphoreSlim instances to free unmanaged resources - Add _disposed flag to prevent double disposal Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
- Pass cancellation token to semaphore wait operations - Enable proper cancellation propagation during initialization Co-authored-by: afshinalizadeh <4254006+afshinalizadeh@users.noreply.github.com>
Fix static field mutation in CrystaProgramSyncModuleService constructor
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
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.