Skip to content

HDDS-11468. Enabled DB sync button - #7216

Merged
ArafatKhan2198 merged 1 commit into
apache:masterfrom
spacemonkd:HDDS-11468
Sep 20, 2024
Merged

ArafatKhan2198 merged 1 commit into
apache:masterfrom
spacemonkd:HDDS-11468

Conversation

@spacemonkd

@spacemonkd spacemonkd commented Sep 18, 2024

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

HDDS-11468. Enabled DB sync button

Please describe your PR in detail:

  • Currently the OM status gets initialized to empty string on component mount. This will cause the disabled status to evaluate to false. But upon clicking the button, the state gets updated and hence the disabled status evaluates to true.
  • When we refresh the page, this state is again reset and the button gets enabled again.
  • The backend will always provide a boolean response and we do not need to check for the empty string
  • In such a scenario even after the initial load we should not disable the button and rather only check the response from backend.
  • This PR makes the change by removing the disable flag allowing the user to click even after the sync.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11468

How was this patch tested?

Patch was tested manually and verified behaviour.

@devmadhuu
devmadhuu self-requested a review September 19, 2024 10:59

@devmadhuu devmadhuu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @devabhishekpal for the patch, Do you know in what scenario, the OM sync response is empty ?

@spacemonkd

Copy link
Copy Markdown
Contributor Author

@devmadhuu, from the backend we will never get empty string.
The empty string is coming from the first initialization of the page. It is initialized to empty string.
Ref:

@devmadhuu

Copy link
Copy Markdown
Contributor

sync button was getting disabled with every click of sync button, not just first click

@spacemonkd

Copy link
Copy Markdown
Contributor Author

Yes Devesh.
So if you check the line: omStatus === '' ? false : true
Ref:

&nbsp;<Button shape='circle' icon={<PlayCircleOutlined />} size='small' loading={isLoading} onClick={omSyncLoad} disabled={omStatus === '' ? false : true} />

This condition would disable the button, if omStatus got a response back i.e the API call was done.

Initially in Overview page we are initializing omStatus: '' which matches the condition and returns true.
Ref:


Hence the button is enabled.

After we click the button omSyncData() is triggered which sets the omStatus state.

Hence the check for disabled now omStatus === '' doesn't match and it returns disabled={true}.

After refresh the state is again being set to '' i.e empty string, and the button is enabled again.

This is the RCA.

We have now removed the disabled prop from the button, thus the button will be enabled at all times allowing the sync.

@devmadhuu

Copy link
Copy Markdown
Contributor

Are we updating tooltip info in this PR as per PR description ?

@spacemonkd

Copy link
Copy Markdown
Contributor Author

We already have that in the code.
Ref:

const omSyncStatusDisplay = omStatus === '' ? '' : omStatus ? <div>OM DB update is successfully triggered.</div> : <div>OM DB update is already running.</div>;

Updated the description to reflect this.

@devmadhuu
devmadhuu self-requested a review September 19, 2024 12:54

@devmadhuu devmadhuu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @devabhishekpal for addressing the comments and explanation. Changes LGTM +1

@ArafatKhan2198 ArafatKhan2198 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested out the changes!
Everything looks good!
Thanks for the patch @devabhishekpal
LGTM +1

@ArafatKhan2198

ghost commented Sep 20, 2024

Copy link
Copy Markdown
Contributor

Thanks for the patch @devabhishekpal
Thanks for the review @devmadhuu
Merging!

@ArafatKhan2198
ArafatKhan2198 merged commit 2b196d1 into apache:master Sep 20, 2024
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.

3 participants