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: LEAP-1651: Properly update meta text for regions #6726

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

hlomzik
Copy link
Collaborator

@hlomzik hlomzik commented Nov 26, 2024

Meta text is saved in regions by Normalization mixin and it's serialized into every result of this region. When we update the meta text we should get this value again from region, not previously stored one in result.

So the priority of sources for meta value was changed. Plus "control meta" was removed.

normInput is also removed, it was an excess field.

PR fulfills these requirements

  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Describe the reason for change

Meta text was not saved during annotation update, because the old saved value took precedence in serialization.

Does this PR introduce a breaking change?

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

It changes behaviour of meta data saving, but that's a rare feature with two data fields saved, easy to track.

What level of testing was included in the change?

  • e2e
  • integration
  • unit

Meta text is saved in regions by Normalization mixin and it's
serialized into every result of this region. When we update the meta text
we should get this value again from region, not previously stored one in result.

So the priority of sources for meta value was changed.
Plus "control meta" was removed.
@github-actions github-actions bot added the fix label Nov 26, 2024
Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for label-studio-docs-new-theme ready!

Name Link
🔨 Latest commit 0189d8a
🔍 Latest deploy log https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/67506cd80638190008feab44
😎 Deploy Preview https://deploy-preview-6726--label-studio-docs-new-theme.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for heartex-docs ready!

Name Link
🔨 Latest commit 0189d8a
🔍 Latest deploy log https://app.netlify.com/sites/heartex-docs/deploys/67506cd88d61ff00084118fc
😎 Deploy Preview https://deploy-preview-6726--heartex-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

It was a model field, so controlled state went through MST update anyway.
And it's saved to actual meta in two places, so we can just get the real value there
and make input uncontrolled.
@MihajloHoma
Copy link
Contributor

MihajloHoma commented Nov 27, 2024

/git merge

Workflow run
Successfully merged: 8 files changed, 67 insertions(+), 13 deletions(-)

Copy link
Contributor

@bmartel bmartel left a comment

Choose a reason for hiding this comment

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

Nice cleanup and context, this will help a lot in furthering our code cleanups!

area.meta is always there, at least as an empty object, so we have to check keys.
@hlomzik hlomzik enabled auto-merge (squash) December 4, 2024 15:00
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.

5 participants