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

[Backport 4.5-7.16] Fix cannot read null properties bug in settings section #5216

Merged
merged 3 commits into from
Mar 10, 2023

Conversation

github-actions[bot]
Copy link
Contributor

Backport 4c113f7 from #5135.

* fixed bug cannot read null properties in settings section

* changelog

(cherry picked from commit 4c113f7)
@github-actions github-actions bot requested a review from a team as a code owner February 17, 2023 16:29
@Desvelao Desvelao linked an issue Feb 20, 2023 that may be closed by this pull request
@Desvelao
Copy link
Member

Desvelao commented Feb 20, 2023

Kibana 7.16.3

Legend:
⚫: none
🟢: pass
🟡: warning
🔴: fail
⚪: not applicable

UI

Test Chrome Firefox Safari
Error is not displayed when going from Settings/Configuration to another section 🟢 🟢
Platform loading indicator is displayed when saving the configuration in Settings/Configuration 🟢

Details

🟢 Error is not displayed when going from Settings/Configuration to another section

Chrome - 🟢
image

Firefox - 🟢
image

Safari - ⚫

🟢 Platform loading indicator is displayed when saving the configuration in Settings/Configuration

Chrome - 🟢
After changing the plugin configuration in Settings/Configuration, the loading in hte header of the plugin platform didn't seem to work.

I was researching this and seems they added a delay time to prevent the uneccessary loadings. Pull request: elastic/kibana#78879

I tried to increase the time to updating the plugin configuration through Settings/Configuration and the loading indicator was displayed. So, it seems to be working as expected.

Moreover, they did some changes in the loading indicator behavior from Kibana 7.10.2 to Kibana 7.16
Kibana 7.10.2 https://github.com/elastic/kibana/blob/v7.10.2/src/core/public/chrome/ui/loading_indicator.tsx#L43-L47
Kibnaa 7.16.3 https://github.com/elastic/kibana/blob/v7.16.3/src/core/public/chrome/ui/loading_indicator.tsx#L35-L45
Kibana 7.17.8 https://github.com/elastic/kibana/blob/v7.17.8/src/core/public/chrome/ui/loading_indicator.tsx#L35-L45

After applying a delay in the time to change the loading indicator status:
image

For another hand, this feature is only presentational and I think it is not a desired functionality. This could be removed in the future if we agree.

Firefox - ⚫

Safari - ⚫

Copy link
Member

@Desvelao Desvelao left a comment

Choose a reason for hiding this comment

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

review

🟢 code
🟢 test #5216 (comment)

@Desvelao
Copy link
Member

The loading indicator when saving the plugin configuration through Settings/Configuration could not be displayed if the time of updating the plugin configuration is lower than 250ms. It is due to in change included in the plugin platform. See the test details in #5216 (comment) for more informaiton.

@github-actions
Copy link
Contributor Author

github-actions bot commented Mar 9, 2023

Code coverage (Jest) % values
Statements 8.65% ( 3275 / 37870 )
Branches 4.44% ( 1300 / 29279 )
Functions 7.51% ( 706 / 9405 )
Lines 8.72% ( 3158 / 36219 )

@yenienserrano
Copy link
Member

yenienserrano commented Mar 10, 2023

Kibana 7.16.3

Legend:
⚫: none
🟢: pass
🟡: warning
🔴: fail
⚪: not applicable

UI

Test Chrome Firefox Safari
Error is not displayed when going from Settings/Configuration to another section 🟢 🟢
Platform loading indicator is displayed when saving the configuration in Settings/Configuration 🟢 🟢

Details

🟢 Error is not displayed when going from Settings/Configuration to another section

Chrome - 🟢

image

Firefox - 🟢

image

Safari - ⚫

🟢 Platform loading indicator is displayed when saving the configuration in Settings/Configuration

Chrome - 🟢

image

Firefox - ⚫

image

Safari - ⚫

Copy link
Member

@yenienserrano yenienserrano left a comment

Choose a reason for hiding this comment

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

@yenienserrano yenienserrano merged commit 63a58f9 into 4.5-7.16 Mar 10, 2023
@yenienserrano yenienserrano deleted the backport-5135-to-4.5-7.16 branch March 10, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read properties of null error in the Settings section
3 participants