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.

Sign up for GitHub

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

Jump to bottom

Fix incorrect pathWithParams for savedFinds. #532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −1
Open

Fix incorrect pathWithParams for savedFinds. #532

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion framework/src/main/groovy/org/moqui/impl/screen/ScreenRenderImpl.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2385,7 +2385,7 @@ class ScreenRenderImpl implements ScreenRender {
for (Map<String, Object> userFlf in userFlfList) {
EntityValue formListFind = (EntityValue) userFlf.formListFind
Map itemMap = [name:formListFind.formListFindId, title:formListFind.description, image:lastImage, imageType:lastImageType,
path:lastPath, pathWithParams:(lastPath + "?formListFindId=" + formListFind.formListFindId)]
path:lastPath, pathWithParams:(currentPath.toString() + (paramString.length() > 0 ? '&' : '?') + "formListFindId=" + formListFind.formListFindId)]
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I think I see what was broken here, in cases with parameters resulting in 2 question mark characters in the pathWithParams.

Small detail, and partly messy due to naming of things above, but it would be helpful to be consistent and use either lastPath or currentPath.toString() in both path and pathWithParams instead of mixed usage. Otherwise the changes look fine, I suppose another way to fix that would be to look for a ? in lastPath.

Copy link
Member Author

Choose a reason for hiding this comment

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

And lastPath is defined before add parameters to currentPath, it doesn't include parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot found a good solution for this. LINE:2399 below this also use lastPath for path without parameters, and currentPath.toString for pathWithParams.
Maybe a utility class can concate query string will improve readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR still have a bug. If paramString already contains a formListFindId, We should remove it first.

if (formListFindId != null && formListFindId.equals(formListFind.formListFindId)) itemMap.active = true
savedFindsList.add(itemMap)
}
Expand Down