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

Share keys #4975

Merged
merged 14 commits into from
Nov 26, 2020
Merged

Share keys #4975

merged 14 commits into from
Nov 26, 2020

Conversation

tephenavies
Copy link
Member

@tephenavies tephenavies commented Nov 8, 2020

What this PR does

Fixes #4774.

Ports shareKeys from version 7.

This PR is now a draft because I haven't updated CHANGES.md and I'm not sure whether to change how share data loading uses upsertModelFromJson while I'm here:

  • It's kinda neat that restoring share data can both update or insert models depending on if they're user-added, or from catalog, but it gets it wrong and tries to insert half a model if the model has been removed from a newer catalog
  • Restoring from share is quite a special thing, so we should maybe do the insert/update & shareKey logic outside of upsertModelFromJson

Apart from that, this functionality should work well in the following scenarios:

  • JSON catalogs (requires implementing a shareKeys adder to catalog-converter)
  • JSON catalog share link against a catalog imported into magda (requires implementing a shareKeys adder to init-to-magda)

Magda catalog share link where the item has moved is partially supported, but only if old share link ids manage to match against shareKeys in parts of the catalog already loaded when the share data is applied.

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 CHANGES.md with what I changed.

@nf-s
Copy link
Contributor

nf-s commented Nov 23, 2020

Revert #4979 after merging this. Related to this issue https://github.com/TerriaJS/qld-digital-twin/issues/237

After reverting close this issue - #4978

Copy link
Contributor

@nf-s nf-s left a comment

Choose a reason for hiding this comment

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

Love it! thanks @steve9164
Just a few small things + Changes.md entry

Copy link
Contributor

@nf-s nf-s left a comment

Choose a reason for hiding this comment

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

I was being too nitpicky - all good
Thanks crispy

@nf-s nf-s marked this pull request as ready for review November 26, 2020 10:10
@nf-s nf-s merged commit 6758c38 into next Nov 26, 2020
@nf-s nf-s deleted the share-keys branch November 26, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants