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

NAS-133904 / 25.10 / Editing the Network Widget saved the values to the other portion of the widget #11524

Merged
merged 3 commits into from
Feb 28, 2025

Conversation

bvasilenko
Copy link
Contributor

Testing:

Please see the ticket for replication steps and expected result.

@bvasilenko bvasilenko requested a review from a team as a code owner February 10, 2025 04:14
@bvasilenko bvasilenko requested review from AlexKarpov98 and removed request for a team February 10, 2025 04:14
@bugclerk
Copy link
Contributor

@bugclerk bugclerk changed the title Editing the Network Widget saved the values to the other portion of the widget NAS-133904 / 25.10 / Editing the Network Widget saved the values to the other portion of the widget Feb 10, 2025
Copy link
Contributor

@AlexKarpov98 AlexKarpov98 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 not able to confirm that it fixed the issue.

Can you explain how this change should fix the bug?

That's what I've seen:

Screen.Recording.2025-02-10.at.16.21.21.mov

@bvasilenko
Copy link
Contributor Author

bvasilenko commented Feb 14, 2025

@AlexKarpov98 Perhaps more details are needed.

I can reproduce the bug at WebUI hosted on the server m50-143a, but using the WebUI built from this PR, the issue does not occur any more, and I see no way how it can occur.

Can you explain how this change should fix the bug?

@AlexKarpov98 The method call sequence was broken. Previously, setValue (line 69) was called before form.valueChanges (line 76) was subscribed. As a result, the form value change was not detected.

image

Copy link
Contributor

@AlexKarpov98 AlexKarpov98 left a comment

Choose a reason for hiding this comment

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

@bvasilenko I see
Could you attach a video where you test is locally, please? (I want to see how it works for you)

@bvasilenko
Copy link
Contributor Author

@AlexKarpov98 here's how it works for me:

simplescreenrecorder-2025-02-13_12.52.01.1.mp4

@undsoft undsoft requested a review from AlexKarpov98 February 20, 2025 00:47
Copy link
Contributor

@AlexKarpov98 AlexKarpov98 left a comment

Choose a reason for hiding this comment

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

It works for me now, issue is resolved magically 👌

Maybe we should do the same for all other widgets?

Screenshot 2025-02-21 at 14 00 20

@AlexKarpov98
Copy link
Contributor

@bvasilenko - yeah, please update in all other places.
I faced the same bug for APPS widget.

I tested using your workaround and it works 👌
I will close the ticket below once it's all merged here.
https://ixsystems.atlassian.net/browse/NAS-134414

@AlexKarpov98 AlexKarpov98 added the backport-25.04.0 Fangtooth Release label Feb 24, 2025
Copy link
Contributor

@AlexKarpov98 AlexKarpov98 left a comment

Choose a reason for hiding this comment

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

👍

@bvasilenko bvasilenko enabled auto-merge (squash) February 28, 2025 05:38
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.36%. Comparing base (024d866) to head (809b465).
Report is 2 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ttings/widget-arbitrary-text-settings.component.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #11524       +/-   ##
===========================================
+ Coverage   60.02%   83.36%   +23.33%     
===========================================
  Files        1655     1655               
  Lines       59255    59255               
  Branches     6409     6409               
===========================================
+ Hits        35568    49397    +13829     
+ Misses      23687     9858    -13829     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bugclerk
Copy link
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Feb 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants