-
-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(typeorm): make the services attribute consistent with mongodb #1111
base: master
Are you sure you want to change the base?
Conversation
* Updated the `getServices` method on the `User` entity to construct objects for the top level fields * Added special handling for `email.verificationTokens` and `password.reset` * Updated tests to match the mongodb tests Fixes: accounts-js#1110
The fix is ugly because I had to add special handling for some hard coded fields, namely I am open to suggestions if there is a better way to do this. The only idea I have in my head right now is, these fields should be in their own tables and I would recommend the same for mongodb as well, since a document has a size limit of 16 MB with the default storage. Since we do not have control over the no: of times a password might get reset for a user or the number of email verification tokens sent out, these might exceed the limits. |
Codecov Report
@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
+ Coverage 95.53% 95.59% +0.05%
==========================================
Files 91 91
Lines 2152 2155 +3
Branches 418 420 +2
==========================================
+ Hits 2056 2060 +4
+ Misses 95 94 -1
Partials 1 1
Continue to review full report at Codecov.
|
Yes totally agree, for mongo this is already in progress #1081 |
@blessanabraham can you add some tests with various services for this function? As I am not super familiar with this part of the code I am afraid of breaking stuff. |
sure, will add some tests if they are missing.I had to modify the tests that I previously added. from mongodb to match the response from typeorm. After this change, the tests. from mongodb and typeorm match up. I tested and made sure that TwoFactor works now with typeorm and after this change. |
Regarding breaking change, no, this change keeps it consistent with what mongodb returns and what the services expect the |
@blessanabraham if you have time to add the tests I can create a release after it's merged with the changes :) |
Hey @pradel , really sorry for the delay on this. Work was a bit hectic. I'll get to it this week |
bdf442b
to
3e57ec8
Compare
@blessanabraham could you please rebase against current master and add some tests so that we can get this in the upcoming 1.0? |
getServices
method on theUser
entity to constructobjects for the top level fields
email.verificationTokens
andpassword.reset
Fixes: #1110