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

Wps results fix #6826

Merged
merged 8 commits into from
Aug 7, 2023
Merged

Wps results fix #6826

merged 8 commits into from
Aug 7, 2023

Conversation

sixlighthouses
Copy link
Contributor

@sixlighthouses sixlighthouses commented Jul 26, 2023

Fix WPS duplicate

Fixes #6392

Workbench items were being duplicated when loaded from WPS results in share URL.
Added a check in models/Terria.ts when loading items to ensure duplicates are not added

Test me

Load this share URL http://ci.terria.io/wps-results-fix/#share=s-uCLMJuXtXBpFXvmJbBI4gE4rGUz

Two items should be loaded to the workbench
image

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@sixlighthouses sixlighthouses marked this pull request as ready for review July 31, 2023 00:19
@na9da
Copy link
Collaborator

na9da commented Aug 4, 2023

Hey @sixlighthouses, that's some good sleuthing 🕵️ job.

The root cause is that our workbench implementation is not protected from duplicate items - that might need a rethink at some point.

The fix you made works, but I would suggest changing newItems to a Set<BaseModel> instead? It'll automatically guard against similar bugs in other code paths. You can then convert the set to array using [...newItems].

@sixlighthouses
Copy link
Contributor Author

thanks @na9da a set is a nice solution will make that change

@na9da
Copy link
Collaborator

na9da commented Aug 4, 2023

Hey @sixlighthouses, on another thought the better fix would be to make the set method for workbench.items sanitize its input by de-duplicating the input array. You could use the lodash uniq method to do that.

Sorry if you already started on the fix.

@sixlighthouses
Copy link
Contributor Author

thanks @na9da updated the workbenches items set method

lib/Models/Terria.ts Outdated Show resolved Hide resolved
@na9da
Copy link
Collaborator

na9da commented Aug 7, 2023

Looks good @sixlighthouses. I have just made a couple of comments.

CHANGES.md Outdated
Comment on lines 3 to 11
#### next release (8.3.2)
#### next release (8.3.3)

- Fixed a bug when restoring timefilter from a share link having more than one imagery item with the same base URL (but different layer names).
- [The next improvement]

#### 8.3.2

- Fix WPS duplicate display of analysis results when loaded through a share URL

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the correct next release for this change is 8.3.2 (so after the timefilter bug fix).

terriajs/CHANGES.md

Lines 3 to 7 in 15d4686

#### next release (8.3.2)
- Fixed a bug when restoring timefilter from a share link having more than one imagery item with the same base URL (but different layer names).
- [The next improvement]

Copy link
Collaborator

@na9da na9da left a comment

Choose a reason for hiding this comment

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

Good to go 🚀

@sixlighthouses sixlighthouses merged commit 8176f13 into main Aug 7, 2023
3 checks passed
@sixlighthouses sixlighthouses deleted the wps-results-fix branch August 7, 2023 04:56
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.

WPS results are showing columns twice after sharing
2 participants