Skip to content

Added unit tests#150

Closed
Patryk-MM wants to merge 10 commits into
the-csharp-academy:mainfrom
Patryk-MM:main
Closed

Added unit tests#150
Patryk-MM wants to merge 10 commits into
the-csharp-academy:mainfrom
Patryk-MM:main

Conversation

@Patryk-MM
Copy link
Copy Markdown

No description provided.

@Dejmenek Dejmenek self-assigned this Apr 30, 2026
@Dejmenek Dejmenek removed their assignment May 12, 2026
@TheCSharpAcademy TheCSharpAcademy self-assigned this May 15, 2026
@TheCSharpAcademy
Copy link
Copy Markdown
Collaborator

@Patryk-MM Project approved! 😄✅ And well done for completing the challenges! 👏 I love the UI/UX. Super smooth and user friendly. You leveraged Spectre Console really well. And you also added unit tests! 🤩

Feedback
🔍️It's good practice to handle errors for every operation, particularly I/O ones such as third-party integrations. A good test is to try to send an e-mail without credentials. Instead of crashing, it would be good to show a meaningful message to the user and let the app keep running:
image

🔍The README says the app uses User Secrets for the database connection string, but the DbContext currently has a hardcoded SQL Server connection string in OnConfiguring. It would be better to inject configuration into the context and read the connection string from configuration/user secrets consistently. Better yet, have a look into the Options pattern for configuration, as it's recommended by Microsoft and it's widely adopted in the industry.

image

🧑‍🏫Overall well done! 👏👏✋🏻Looking forward to seeing your next projects!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants