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

fix(webapi): unique username/email check on change user #1561

Merged
merged 19 commits into from Jun 25, 2020

Conversation

@LukasStoeckli
Copy link
Contributor

@LukasStoeckli LukasStoeckli commented Dec 17, 2019

No description provided.

@LukasStoeckli LukasStoeckli changed the title test(webapi): add unique username/email check to change user fix(webapi): unique username/email check on change user Dec 23, 2019
@LukasStoeckli LukasStoeckli requested a review from subotic Dec 23, 2019
LukasStoeckli and others added 5 commits Jan 7, 2020
@subotic
Copy link
Collaborator

@subotic subotic commented Apr 20, 2020

@LukasStoeckli what is the status of this PR?

@LukasStoeckli
Copy link
Contributor Author

@LukasStoeckli LukasStoeckli commented Apr 21, 2020

works, but:

  • 'check if current email/username differs from the one in the change request' not in place, as I could not figure out how this nested conditions and exceptions stuff works
  • stringFormatter.validate... from develop needs to be merged
@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Apr 21, 2020

I haven't looked closely at this, but if you need to lock something to guarantee uniqueness of the values, you can use the Knora utility class IriLocker instead of Akka's Await.

@LukasStoeckli
Copy link
Contributor Author

@LukasStoeckli LukasStoeckli commented Apr 21, 2020

there is a global IRI lock. The point would be not to perform an update if the value hasn't changed. The issue is the scope of the conditions respectively the exceptions thrown in it. As in the code on develop, the exception is thrown but doesn't lead to not executing the rest of the function. As in this PR, everything works as intended, but there's no longer a check whether the new value is actually new.

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Apr 21, 2020

the exception is thrown but doesn't lead to not executing the rest of the function

I don't see how that's possible. Are you sure? is there a test for that?

@LukasStoeckli
Copy link
Contributor Author

@LukasStoeckli LukasStoeckli commented Apr 21, 2020

me neither. If you run these without the changes in UsersResponderADM, the values are 'successfully' updated.

@benjamingeer benjamingeer self-requested a review Apr 22, 2020
@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Apr 28, 2020

Is the failing ResourcesRouteV2E2E a common issue in the CI?

Yes, GraphDB often runs out of memory in GitHub CI, but we don't know why.

LukasStoeckli and others added 2 commits Apr 29, 2020
@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Jun 2, 2020

ping

benjamingeer added 5 commits Jun 4, 2020
@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Jun 25, 2020

@subotic Should we merge this PR?

@subotic
Copy link
Collaborator

@subotic subotic commented Jun 25, 2020

@benjamingeer I guess so. The tests are passing.

@benjamingeer benjamingeer merged commit 4f26e22 into develop Jun 25, 2020
7 checks passed
7 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
Publish to Dockerhub (on release only)
Details
@benjamingeer benjamingeer deleted the wip/fix-change-user branch Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants