Skip to content
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

Merged
merged 14 commits into from
May 24, 2024

Conversation

Drakonian
Copy link
Contributor

@Drakonian Drakonian commented Mar 9, 2024

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

…ch the OldPassword

Also in this case we check that the old password matches the entered old password microsoft#328
@Drakonian Drakonian requested a review from a team as a code owner March 9, 2024 18:59
@github-actions github-actions bot added AL: System Application From Fork Pull request is coming from a fork labels Mar 9, 2024
@Drakonian Drakonian requested a review from grobyns March 11, 2024 18:49
@Drakonian
Copy link
Contributor Author

PR is updated

@JesperSchulz
Copy link
Contributor

Build is failing. Please update PR and let's do another run 😊

@Drakonian
Copy link
Contributor Author

Updated

@JesperSchulz JesperSchulz self-assigned this Mar 13, 2024
@JesperSchulz JesperSchulz added the Linked Issue is linked to a Azure Boards work item label Mar 13, 2024
JesperSchulz
JesperSchulz previously approved these changes Mar 13, 2024
JesperSchulz
JesperSchulz previously approved these changes Mar 13, 2024
@JesperSchulz JesperSchulz enabled auto-merge (squash) March 13, 2024 14:58
@JesperSchulz
Copy link
Contributor

I'll do some functional testing of those changes early next week 🙂 If all goes well, we merge!

auto-merge was automatically disabled March 25, 2024 14:04

Head branch was pushed to by a user without write access

JesperSchulz
JesperSchulz previously approved these changes May 21, 2024
@JesperSchulz
Copy link
Contributor

@Drakonian, test compilation now seems to fail. Will you investigate?

@Drakonian
Copy link
Contributor Author

@JesperSchulz

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.

@grobyns
Copy link
Contributor

grobyns commented May 21, 2024

@JesperSchulz

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 :-)

auto-merge was automatically disabled May 21, 2024 15:12

Head branch was pushed to by a user without write access

@Drakonian
Copy link
Contributor Author

@JesperSchulz
Ready to check

JesperSchulz
JesperSchulz previously approved these changes May 22, 2024
@JesperSchulz JesperSchulz enabled auto-merge (squash) May 22, 2024 13:09
@Drakonian
Copy link
Contributor Author

@JesperSchulz broken builder?

image

id looks correct

image

@Drakonian
Copy link
Contributor Author

image

@JesperSchulz
Copy link
Contributor

@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.

auto-merge was automatically disabled May 22, 2024 17:24

Head branch was pushed to by a user without write access

@Drakonian Drakonian requested a review from a team as a code owner May 22, 2024 17:24
@Drakonian
Copy link
Contributor Author

Drakonian commented May 22, 2024

@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.

Corrected to old ID range.
Do you have any idea why this is compiling locally? 😮

@JesperSchulz
Copy link
Contributor

@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.

Corrected to old ID range. Do you have any idea why this is compiling locally? 😮

You are probably only compiling the entire System Application in one go, and not the modules individually - is my guess 🙂

@Drakonian
Copy link
Contributor Author

@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.

Corrected to old ID range. Do you have any idea why this is compiling locally? 😮

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 😞
I hope you're not wasting too much time on this

new correction ready, should be last 😈

@JesperSchulz
Copy link
Contributor

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.

@JesperSchulz JesperSchulz enabled auto-merge (squash) May 24, 2024 08:33
@JesperSchulz JesperSchulz merged commit 6e0b1ac into microsoft:main May 24, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AL: System Application From Fork Pull request is coming from a fork Integration GitHub request for Integration area Linked Issue is linked to a Azure Boards work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event Request on Changing Password
5 participants