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

feat: removed dashboard uuid is all cases be it duplicate, empty or somevalid, while import json #6448

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

SagarRajput-7
Copy link
Contributor

@SagarRajput-7 SagarRajput-7 commented Nov 15, 2024

Summary

Here we want to remove uuid for people in case when they are empty string or duplicate, but for identifying a duplicate we would need an api call which is expensive, hence we decided to rather remove uuid (exisiting) in all cases, and later once the importing is done, we from behind the scene will provide each dashboard with a unique uuid.

With this we wont block user for import json jsut on the uuid parameters

Related Issues / PR's

Screenshots

Screen.Recording.2024-11-15.at.7.17.03.PM.mov

Affected Areas and Manually Tested Areas

  • Tested import json flows with -
    • uuid present duplicate or valid
    • uuid empty string
    • uuid not present

Important

Remove uuid from dashboardData during JSON import in ImportJSON component, regardless of its value.

  • Behavior:
    • In ImportJSON component, uuid is removed from dashboardData during JSON import, regardless of its value (duplicate, empty, or valid).

This description was created by Ellipsis for 8778934. It will automatically update as commits are pushed.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 8778934 in 20 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/container/ListOfDashboard/ImportJSON/index.tsx:86
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values for consistency. This applies to twoToneColor in line 145 and style in line 146.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is not related to the changes made in the diff, which are about the validation of uuid. The comment refers to lines 145 and 146, which are not part of the diff changes. Therefore, the comment is not about a change made in this diff.
    I might be missing the context of the entire file, but the task is to focus on the diff changes. The comment does not address the changes made in the diff.
    The task is to focus on the diff changes, and the comment does not address those changes. Therefore, it should be removed.
    The comment is not about the changes made in the diff, so it should be deleted.

Workflow ID: wflow_VFC6V0co1Uz4xqvK


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

vikrantgupta25
vikrantgupta25 previously approved these changes Nov 15, 2024
Copy link
Collaborator

@vikrantgupta25 vikrantgupta25 left a comment

Choose a reason for hiding this comment

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

Explain above what is the validation of the uuid, the comments is non intuitive

@SagarRajput-7
Copy link
Contributor Author

Explain above what is the validation of the uuid, the comments is non intuitive

yes was about to update them.

@YounixM
Copy link
Member

YounixM commented Nov 15, 2024

@SagarRajput-7 : I am not sure about the functionality of this PR and why this is required. Can you add details around it please.

makeavish
makeavish previously approved these changes Nov 15, 2024
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 713b93b in 9 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/container/ListOfDashboard/ImportJSON/index.tsx:85
  • Draft comment:
    The comment can be simplified to better reflect the implementation:
// Remove uuid from the dashboard data if it exists
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The current implementation deletes the uuid property from dashboardData if it exists. This is consistent with the PR description, which states that the uuid should be removed regardless of its value. However, the comment above the code could be more concise and aligned with the implementation.

Workflow ID: wflow_i8o1uhF4vbuHI3Bd


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@SagarRajput-7 SagarRajput-7 merged commit c1478c4 into develop Nov 15, 2024
13 of 15 checks passed
@SagarRajput-7 SagarRajput-7 deleted the handle-duplicate-uuid branch November 15, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs not required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants