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

Link component change in href rendering behaviour without applicable unit test #1909

Open
mg-aceik opened this issue Aug 28, 2024 · 5 comments
Labels
backlog Issue/PR/discussion is reviewed and added to backlog for the further work 🐞 bug

Comments

@mg-aceik
Copy link

Describe the Bug

The change here b4309e8#diff-cd0f8f166dc72a9504e6cf7b0dfaaf409561f62d651b115c3e8185151162c7e1R36 in JSS 21.1.0 is a breaking change in that it needs a metadata property otherwise it will not render the link.

There isn't actually any unit test that is asserting this - e.g. you could have a field with a value and href, but if no metadata is set then it still returns null. I'm not sure if that was intentional or not and so I think a unit test should be added to assert that it was intentional.

To Reproduce

N/A

Expected Behavior

N/A

Possible Fix

N/A

Provide environment information

  • Sitecore Version: XM Cloud
  • JSS Version: 22.1.0
@illiakovalenko
Copy link
Contributor

@mg-aceik Hey
So this "if" statement is executed and returns null only in case if field is empty OR in case if value AND editable AND href AND metadata are not present. In this case if you have the field that contains value or href but doesn't have metadata it will not return null. Can you, please, elaborate more on this?

  if (
      !field ||
      (!(field as LinkFieldValue).editable &&
        !field.value &&
        !(field as LinkFieldValue).href &&
        !field.metadata)
    ) {
      return null;
    }

@mg-aceik
Copy link
Author

Hi @illiakovalenko

Ok you're right I saw that my links that have a text value and no href are no longer rendering which was a change in behaviour from 22.0.0. I thought it was the change to the metadata field check but it's not, it must be something else, but I can't see what.

I've added unit tests here: https://github.com/mg-aceik/jss/pull/1/files
If you run those tests I added against the 22.1.0 then they all pass.
If you run those tests I added against the 22.0.0 then you get this failure:

should render nothing when field is present and href is not present:

  AssertionError: expected '<a href="undefined">ipsum</a>' to equal ''
  + expected - actual

  -<a href="undefined">ipsum</a>

@mg-aceik mg-aceik changed the title Link component metadata change in behaviour without applicable unit test Link component change in href rendering behaviour without applicable unit test Aug 29, 2024
@art-alexeyenko
Copy link
Contributor

Hi @mg-aceik

I registered a bug for our backlog for this. Can you help us in prioritizing it, if you have a detailed repro scenario?
Currently I see this happening if serialization went wrong, or if link raw values were modified directly. Are there situation when an editor can accidentally trigger this?

@mg-aceik
Copy link
Author

mg-aceik commented Sep 5, 2024

This was just something my unit tests picked up for my components that were using links. I am ok with the change, I think, but just saw that there wasn't any unit test your side to cover the change, as I presume your team made this change to fix something but it has no unit test so it feels like it could be unintentional.

@illiakovalenko illiakovalenko added the backlog Issue/PR/discussion is reviewed and added to backlog for the further work label Sep 5, 2024
@art-alexeyenko
Copy link
Contributor

@mg-aceik it is unintentional indeed, and more of a case that hasn't crossed our our field of view. We've added an item to backlog to address this, fortunately it's not a severe bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issue/PR/discussion is reviewed and added to backlog for the further work 🐞 bug
Projects
None yet
Development

No branches or pull requests

3 participants