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 app crash when replacing views with object ref from graph.findOrAddObject() #405

Merged
merged 2 commits into from
Jun 22, 2022

Conversation

thinkh
Copy link
Member

@thinkh thinkh commented Jun 15, 2022

Fixes #402

Developer Checklist (Definition of Done)

  • Descriptive title for this pull request provided (will be used for release notes later)
  • All acceptance criteria from the issue are met
  • Branch is up-to-date with the branch to be merged with, i.e., develop
  • Code is cleaned up and formatted
  • Code documentation written
  • Unit tests written
  • Tested in Chrome
  • Tested in Firefox
  • Build is successful
  • Summary of changes written
  • Wiki documentation written
  • Assign at least one reviewer
  • Assign at least one assignee
  • Add type label (e.g., bug, enhancement) to this pull request
  • Add next version label (e.g., next-version: minor) to this PR following semver

Summary of changes

Screenshots

ordino-replace-view-findOrAddObject

Fixes #402

See GitHub issue for in-depth analysis and testing results.
@thinkh thinkh requested a review from oltionchampari June 15, 2022 20:27
@thinkh thinkh added type: bug Something isn't working release: patch PR merge results in a new patch version labels Jun 15, 2022
@thinkh thinkh changed the title Fix object ref with graph.findOrAddObject() when replacing views Fix app crash when replacing views with object ref from graph.findOrAddObject() Jun 15, 2022
@thinkh
Copy link
Member Author

thinkh commented Jun 20, 2022

@rumersdorfer I've tested this PR with Ordino Public and it worked as before. However, it would be good to have a second opinion and testing cycle for Ordino. Please test Ordino Daily against a local Ordino instance with this PR. Check out the following branches in your local Ordino workspace:

image

Test if all actions regarding opening, closing and replacing a second view are still working as expected and if old provenance tracks (i.e., exported from Ordino Daily and imported locally) still result in the same state.

Please post your result here and if everything works submit a successful review. Let me know if you have further questions. Thanks.

@thinkh thinkh requested a review from rumersdorfer June 20, 2022 14:53
Copy link
Contributor

@oltionchampari oltionchampari left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed explanation. The bug is now fixed. Tested also replaying provenance graphs generated from the dev branch and I saw no side effects.
From my side, you can go ahead and merge.

@dg-datavisyn dg-datavisyn merged commit 0e1ec35 into develop Jun 22, 2022
@dg-datavisyn dg-datavisyn deleted the thinkh/402-fix-replace-view-object-ref branch June 22, 2022 09:09
@rumersdorfer
Copy link
Contributor

@thinkh @oltionchampari Bug is fixed in Indications. 👍

@rumersdorfer
Copy link
Contributor

Bug is also fixed in Ordino. 👍

@dvvanessastoiber dvvanessastoiber mentioned this pull request Aug 4, 2022
2 tasks
@thinkh thinkh mentioned this pull request Jul 28, 2023
dvvanessastoiber added a commit that referenced this pull request Jul 28, 2023
## What's Changed

### Features
* CLUE support for URL query parameter + refactor public session link by
@thinkh in #419
* Replace jQuery scrollTo plugin with custom implementation by @thinkh
in #425
* Improve typings for new session functions by @thinkh in
#427
* Allow tour card description to include html by @oltionchampari in
#432

### Bugfixes

* Fix wrong view name in replace action title by using `ViewWrapper`
context by @thinkh in #406
* Fix app crash when replacing views with object ref from
`graph.findOrAddObject()` by @thinkh in
#405
* Add vertical padding to tour cards and use full height teaser image by
@dvmichaelpeterseil in #430

### Chore

* Remove `nav.mainNavi` styles by @thinkh in
#415
* Improve Cypress setup and use Cypress commands.js by @dvvanessastoiber
in #354
* Add Visyn Scripts by @datavisyn-bot in
#421
* Add dist/ folder by @dvtschachinger in
#423
* Migration to visyn_core by @oltionchampari in
#429
* React 18 Migration by @dvmoritzschoefl in
#431

## New Contributors
* @datavisyn-bot made their first contribution in
#421
* @dvtschachinger made their first contribution in
#423
* @dvmichaelpeterseil made their first contribution in
#430
* @dvmoritzschoefl made their first contribution in
#431

**Full Changelog**:
v13.0.1...v14.0.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: patch PR merge results in a new patch version type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants