-
Notifications
You must be signed in to change notification settings - Fork 172
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
OpenChangePasswordDialog new rules #328 #714
OpenChangePasswordDialog new rules #328 #714
Conversation
…ch the OldPassword Also in this case we check that the old password matches the entered old password microsoft#328
PR is updated |
src/System Application/App/Password/src/PasswordDialogImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
Build is failing. Please update PR and let's do another run 😊 |
Updated |
src/System Application/App/Password/src/PasswordDialogImpl.Codeunit.al
Outdated
Show resolved
Hide resolved
I'll do some functional testing of those changes early next week 🙂 If all goes well, we merge! |
src/System Application/Test/Password/src/PasswordDialogTest.Codeunit.al
Outdated
Show resolved
Hide resolved
src/System Application/Test/Password/src/PasswordDialogTest.Codeunit.al
Outdated
Show resolved
Hide resolved
Head branch was pushed to by a user without write access
@Drakonian, test compilation now seems to fail. Will you investigate? |
Yes of course, the problem is that Test Password is a separate app and it cannot access internal implementation from System Application. The strangest thing is that I didn't get this error in testing locally, probably the build system in the project is still not perfect. I will now update the test that refers to internal codeunit. |
FYI: tests should not have direct access to internals. If some internals access is needed, then it should be through a test library app which encapsulates the internal and exposes some public method allowing what you need. If you can avoid using internals completely, that's great, if not, please add methods to a test library :-) |
…akonian/BCApps into PasswordDialogAdditionalRules
Head branch was pushed to by a user without write access
@JesperSchulz |
@JesperSchulz broken builder? id looks correct |
@Drakonian, no, there's still a permission set with ID 135033. Now that you've changed the app.json, that ID is outside of the ID range - hence the compilation error. Add the range from 135033 to 135033, and that issue should be gone. |
Head branch was pushed to by a user without write access
Corrected to old ID range. |
You are probably only compiling the entire System Application in one go, and not the modules individually - is my guess 🙂 |
I feel a little confused about having to run the CI process several times and it finds new errors. It would be cool to run it locally somehow 😞 new correction ready, should be last 😈 |
No worries. I just click a button 🙂 It's perfectly normal that you need to submit several updates before builds succeed. Of course you can (and should) enable all code analyzers with the correct rules, but things like compiling the modules individually or running CLEAN builds, is too much to ask. We all here in the BC team use CI to verify those things. |
Summary
If OldPassword is passed, we check that the new password should match the OldPassword
Also in this case we check that the old password matches the entered old password
New test
Work Item(s)
Fixes #328
Fixes AB#506715