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

Issue #SH-135 managedby-user::update scenario - when email/phone is u… #949

Open
wants to merge 3 commits into
base: release-3.0.0
Choose a base branch
from

Conversation

Hari-stackroute
Copy link
Collaborator

…pdated, managedBy flag set to null

when email/phone is updated for a managedBy user, then managedBy filed is set to null.
Response:
{ "id": "api.user.update", "ver": "v1", "ts": "2020-05-19 12:07:34:772+0530", "params": { "resmsgid": null, "msgid": null, "err": null, "status": "success", "errmsg": null }, "responseCode": "OK", "result": { "response": "SUCCESS", "link": "reset_password_link.html" } }

@Hari-stackroute Hari-stackroute requested a review from indrajra May 19, 2020 10:53
@@ -963,6 +987,25 @@ private void sendEmailAndSms(Map<String, Object> userMap) {
tellToAnother(EmailAndSmsRequest);
}

private Response sendResetPasswordLink(Map<String, Object> userMap) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this already somewhere? If so, please remove that and unify these code.

Copy link
Collaborator Author

@Hari-stackroute Hari-stackroute May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I searched no-where in our code base this is present. Normally from ResetPasswordController "resetPassword" actor is called. In this use-case we need reset-password link to be generated to called from here.

LoggerEnum.INFO.name());
Response response =
(Response)
interServiceCommunication.getResponse(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bad idea - interServiceCommunication within the actor systems. Please get rid of it and call the actor directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indrajra if we want to call a actor from another actor, we are calling tellToAnother() in the BaseActor, but whatever actor we call from this is purely async calls. Here in my scenario sync call is needed so tellToAnother() cannot be used. Even InterServiceCommunicationImpl.getResponse also calling the actor the same way you suggested. Correct me if i am wrong.

@@ -901,6 +916,15 @@ private void setStateValidation(
userBooleanMap.put(
JsonKey.STATE_VALIDATED, (boolean) userDbRecord.get(JsonKey.STATE_VALIDATED));
}
// adding in release-3.0.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without email, phone, setting these fields doesnt make sense. If there is any failure due to it, lets fix that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indrajra email and phone are part of it.
This is suggested in the ticket

@@ -206,6 +207,12 @@ private void updateUser(Request actorMessage) {
Map<String, Boolean> userBooleanMap = updatedUserFlagsMap(userMap, userDbRecord);
int userFlagValue = userFlagsToNum(userBooleanMap);
requestMap.put(JsonKey.FLAGS_VALUE, userFlagValue);
if (StringUtils.isNotEmpty((String) userDbRecord.get(JsonKey.MANAGED_BY))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment - to who will this apply? A regular user or managed user.
A managed user cannot update an email or phone (UI will not allow it).
If it happens in future, then we will accept it and set managedBy = null. This managed user has become an adult and is no more dependent on the regular user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indrajra the following is suggested in the ticket .

  • If an email or phone is added to a user that has a non-null ‘managedBy’, do the following. We shall consider these no more as managed users.
  • generate a reset password link

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

41.4% 41.4% Coverage
0.0% 0.0% Duplication

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.

2 participants