fix(typeorm): make the services attribute consistent with mongodb #1111
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 |
darkbasic
force-pushed
the
master
branch
9 times, most recently
from
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? |
getServicesmethod on theUserentity to constructobjects for the top level fields
email.verificationTokensandpassword.resetFixes: #1110