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 incorrect pathWithParams for savedFinds. #532

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

Conversation

chunlinyao
Copy link
Member

Must include parameters for screen.

Must include parameters for screen.
@@ -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.

@chunlinyao chunlinyao force-pushed the master branch 2 times, most recently from e8e881a to 5bd151b Compare May 23, 2023 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants