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

Feature 7434 do not validate case save from visit #7556

Merged

Conversation

BarnaBartha
Copy link
Contributor

@BarnaBartha BarnaBartha commented Dec 21, 2021

Fixes #7434

Copy link
Contributor

@MateStrysewske MateStrysewske left a comment

Choose a reason for hiding this comment

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

You would've spared yourself a lot of refactoring work by adding a second saveCase method with the additional parameter, and by extending the current method to call the new method with false :) I'd suggest to still do this because it makes the code a lot cleaner (i.e. far less unnecessary parameters).

@BarnaBartha
Copy link
Contributor Author

Actually the IDE changed the signature automatically so no trouble there.
But I do agree that exposing it that way shows the actual problem in an enhanced way, that there is no true separation or the concept of system action vs user action. It is a specific flag for this scenario, and after some time a similar flag will appear for another entity's saving/validation logic and we'll just increase the unnecessary logic that complicates developers lives later as we never truly fix these issues.

@@ -1335,7 +1335,12 @@ public CaseReferenceDto getReferenceByUuid(String uuid) {

@Override
public CaseDataDto saveCase(@Valid CaseDataDto dto) throws ValidationRuntimeException {
return saveCase(dto, true, true);
return saveCase(dto, true, true, false, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

internal should not be set to false here; see line 1455, the method there sets it to true as well. If I understand it correctly, internal should always be true unless the case comes from an external source (e.g. sormas2sormas). I suggest to ommit the argument here and let the saveCase method in 1455 handle it.


@Override
public CaseDataDto saveCase(@Valid CaseDataDto dto, Boolean systemSave) throws ValidationRuntimeException {
return saveCase(dto, true, true, false, systemSave);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@@ -3167,7 +3172,7 @@ private void mergeCase(CaseDataDto leadCaseData, CaseDataDto otherCaseData, bool
// 1.1 Case

copyDtoValues(leadCaseData, otherCaseData, cloning);
saveCase(leadCaseData, !cloning, true);
saveCase(leadCaseData, !cloning, true, false, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@@ -77,7 +77,7 @@
@POST
@Path("/push")
public List<PushResult> postCases(@Valid List<CaseDataDto> dtos) {
return savePushedDto(dtos, FacadeProvider.getCaseFacade()::saveCase);
return savePushedDto(dtos, dto -> FacadeProvider.getCaseFacade().saveCase(dto));
Copy link
Contributor

Choose a reason for hiding this comment

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

Strange refactoring - the :: notation should actually be the preferred one by the IDE

@MateStrysewske MateStrysewske merged commit 425af76 into development Jan 6, 2022
@MateStrysewske MateStrysewske deleted the feature-7434-DoNotValidateCaseSaveFromVisit branch January 6, 2022 09:55
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.

Visits for contacts cannot be saved if users don't have access to the converted case
2 participants