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

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
@subotic
Copy link
Collaborator

subotic commented Apr 20, 2020

@LukasStoeckli what is the status of this PR?

@LukasStoeckli
Copy link
Contributor Author

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

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

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

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

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

@benjamingeer benjamingeer self-requested a review April 22, 2020 08:51
@benjamingeer
Copy link

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.

@benjamingeer
Copy link

ping

@benjamingeer
Copy link

@subotic Should we merge this PR?

@subotic
Copy link
Collaborator

subotic commented Jun 25, 2020

@benjamingeer I guess so. The tests are passing.

@benjamingeer benjamingeer merged commit 4f26e22 into develop Jun 25, 2020
@benjamingeer benjamingeer deleted the wip/fix-change-user branch June 25, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants