483/shav-mailjet-stage-segments #329
Open
Conversation
This file contains 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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Extend updateUserProperties method to include stage parameter - Add stage to the JSON data sent in Mailjet API request - Ensure consistent formatting and error handling This change allows for updating user stage information in Mailjet.
- Verify ExternalAccountManager throws ExternalAccountSynchronisationException when MailjetException occurs during account retrieval - Test covers scenario of Mailjet API communication failure - Ensures proper error propagation in synchroniseChangedUsers method
src/test/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapperTest.java
Outdated
Show resolved
Hide resolved
src/test/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapperTest.java
Outdated
Show resolved
Hide resolved
- Implement tests for getAccountByIdOrEmail and addNewUserOrGetUserIfExists methods - Use EasyMock for mocking MailjetClient and MailjetResponse - Include test for handling null email input - Set up test environment with mock injection in setUp method
- Add new utility method `invokePrivateMethod` to ReflectionUtils - Extend ReflectionUtilsTest with additional test cases - Improve test coverage for getClasses and getSubTypes methods - Ensure consistent naming and structure across test methods - Update documentation for ReflectionUtils class This commit enhances the capabilities of our reflection utilities and strengthens our test suite, improving overall code quality and maintainability.
- Create PgExternalAccountPersistenceManagerTest class - Implement test for buildUserExternalAccountChanges method - Use ReflectionUtils to access private method for testing - Mock ResultSet to simulate database interaction - Verify correct creation of UserExternalAccountChanges object This commit improves test coverage for the PgExternalAccountPersistenceManager class, ensuring the correct handling of user account data from the database.
src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| void synchroniseChangedUsers_existingUser() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { |
There was a problem hiding this comment.
I'm struggling a little to see what the difference in behaviour between the existing and newuser cases are. Could you insert a comment or something in the test data to make that clearer?
| @@ -102,6 +103,9 @@ public void updateExternalAccount(final Long userId, final String providerUserId | |||
| } | |||
|
|
|||
| private UserExternalAccountChanges buildUserExternalAccountChanges(final ResultSet results) throws SQLException { | |||
| String registeredContextsJson = results.getString("registered_contexts"); | |||
There was a problem hiding this comment.
Is this all working OK in your local database? It looks like it requires db schema changes to include the registered contexts.
I might be wrong - I don't remember how that information was stored when we looked at it
- Updated test expectations in PgExternalAccountPersistenceManagerTest to align with method behavior - Correctly mock ResultSet to handle registered_contexts JSON data - Added assertions to verify proper JSON parsing and stage extraction - Removed invalid expectations for ResultSet.getString() calls - Improved test readability and ensured consistency with the method logic These changes resolve mismatched expectations and improve the accuracy of the tests, ensuring proper handling of registered_contexts JSON data
|
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.
Description:
This PR introduces a new
invokePrivateMethodutility in ourReflectionUtilsclass, enabling safer testing of private methods. I demonstrated its use by implementing comprehensive unit tests forPgExternalAccountPersistenceManager, focusing on thebuildUserExternalAccountChangesmethod. Then, I updated this method to properly parse JSON data for registered contexts, improving our handling of user account information. The changes include new test methods inReflectionUtilsTestand a complete test suite forPgExternalAccountPersistenceManager. These enhancements could significantly improve our test coverage and ensure more robust processing of user data from the database.Ticket: https://github.com/isaaccomputerscience/isaac-cs-issues/issues/483
Media Attachement(s):
Screenshot of all of the unit test suite passing
A screen recording demonstrating the steps from the Mailjet Documentation on how to create separate segments for A-Level and GCSE students and setting up distinct segments using the
"stage"property..Screen.Recording.2025-01-03.at.12.03.08.mov