-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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] Sync of non existent field throws exception #8006
[FIX] Sync of non existent field throws exception #8006
Conversation
Awesome @goiaba Can you create unit tests for this method? You can see some tests here https://github.com/RocketChat/Rocket.Chat/blob/develop/packages/rocketchat-markdown/tests/client.tests.js You can run using |
This has me worried. Does the existing behavior of Rocket.Chat allow dots in the customField name and it be treated as a regular field and not nested ones? If so, then this is a breaking change... |
@graywolf336 The existing behavior is the same, the dot is allowed, the difference now is that you can access sub values of the LDAP object |
@rodrigok Sure, I'll do it. @graywolf336 Don't worry! They were already treated as nested fields in customFields (I was just emphasizing). |
@rodrigok I'm in trouble trying to create the unit tests. Following the example you gave me, I'm not able to import the mut in the test file. It complains that "ReferenceError: Logger is not defined". If I also import the Logger from Any tips? |
@goiaba, I'll try to help you with the unit tests. But any tips from any of the core team members would be great. |
@goiaba can you fix the conflicts? |
@goiaba Can you show them the problem that we were getting when executing the unit test? That error that occured because of some problem on package |
- checks if the field is whitelisted for sync - if a customField, checks its validity before syncing (through Accounts_CustomFields) - fixes the case when the field is still absent in the user document * This implementation does not allow dots in the customField name, because the dots would be treated as nested fields
7ffcfe0
to
8601754
Compare
Good idea! @vinimdocarmo oriented me to follow this guide So I added to Package.onTest(function(api){
api.use('rocketchat:ldap');
api.use('rocketchat:ldapjs');
api.use('rocketchat:logger');
api.use('rocketchat:lib');
api.use('yasaricli:slugify');
api.use('ecmascript');
api.use('sha');
api.use(['ecmascript', 'random', 'practicalmeteor:mocha']);
api.mainModule('tests/sync.tests.js');
}); Then I tried to run the tests this way: [10:08:53] bruno@ideapad-Y700:~/CloudStorage/GitHub/Rocket.Chat$ meteor test-packages ./packages/rocketchat-ldap/ But the result was: [[[[[ Tests ]]]]]
=> Started proxy.
=> Started MongoDB.
=> Errors prevented startup:
While processing files with ecmascript (for target web.browser):
packages/rocketchat:streamer/client/client.js:109:2: super() outside of class constructor (109:2)
While processing files with ecmascript (for target os.linux.x86_64):
packages/rocketchat:streamer/server/server.js:253:3: 'super' outside of function or class (253:3)
=> Your application has errors. Waiting for file change. |
@RocketChat/core