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

UI implementation for allowing to have api policy and a common policy with same name and same version #809

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RusJaI
Copy link
Contributor

@RusJaI RusJaI commented Oct 15, 2024

@RusJaI RusJaI marked this pull request as draft October 15, 2024 11:56
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
38.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@RusJaI RusJaI marked this pull request as ready for review December 24, 2024 20:21
@RusJaI RusJaI changed the title [DRAFT]UI implementation for allowing to have api policy and a common policy with same name and same version UI implementation for allowing to have api policy and a common policy with same name and same version Dec 24, 2024
setDrawerOpen,
PolicyConfigurationEditDrawer
}) => {
const AttachedPolicyCardShared: FC<AttachedPolicyCardSharedProps> = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we revert back to the prop destructuring here?

Suggested change
const AttachedPolicyCardShared: FC<AttachedPolicyCardSharedProps> = (props) => {
const AttachedPolicyCardShared: FC<AttachedPolicyCardSharedProps> = ({
...
}) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was introduced based on the following considerations:

Data flows through this shared component to both the Global Policy UIs and the API/Common Policy UIs. In the context of Global Policy flows, the concept represented by listOriginatedFromCommonPolicies does not apply, and as a result, this prop is not passed. Furthermore, in the API/Common Policy flows, there are scenarios where listOriginatedFromCommonPolicies may not exist.

Due to the lack of a definitive way to distinguish between a Global Policy page and a standard policy page based solely on the value of listOriginatedFromCommonPolicies in the props, destructuring was removed. Instead, a check was introduced to verify whether the corresponding prop exists among the passed props, rather than relying on its value.

Comment on lines +85 to +86
const policyColor = Utils.stringToColor(props.policyObj.displayName);
const policyBackgroundColor = props.drawerOpen
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not do these modifications. Check and fix similar places as well.

Suggested change
const policyColor = Utils.stringToColor(props.policyObj.displayName);
const policyBackgroundColor = props.drawerOpen
const policyColor = Utils.stringToColor(policyObj.displayName);
const policyBackgroundColor = drawerOpen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relates to #809 (comment)

title={`${policyObj.displayName} : ${policyObj.version}`}
placement='top'

if ('listOriginatedFromCommonPolicies' in props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we specify this as a prop passed down from the parent component instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relates to #809 (comment)


if ('listOriginatedFromCommonPolicies' in props) {
// Props were passed, use `listOriginatedFromCommonPolicies` and `isApiRevision`
let policyType = "Common Policy";
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we make Common Policy and API Policy constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
}
return (
(<Root>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we remove this (?

Suggested change
(<Root>
<Root>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

// Combine the two using a union type
type AttachedPolicyListSharedProps = AttachedPolicyListWithCommonProps | AttachedPolicyListWithoutCommonProps;

const AttachedPolicyListShared: FC<AttachedPolicyListSharedProps> = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we destruct the props here?

Suggested change
const AttachedPolicyListShared: FC<AttachedPolicyListSharedProps> = (props) => {
const AttachedPolicyListShared: FC<AttachedPolicyListSharedProps> = ({
...
}) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relates to #809 (comment)

type AttachedPolicyListSharedProps = AttachedPolicyListWithCommonProps | AttachedPolicyListWithoutCommonProps;

const AttachedPolicyListShared: FC<AttachedPolicyListSharedProps> = (props) => {
if ('listOriginatedFromCommonPolicies' in props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relates to #809 (comment)

Comment on lines +77 to +85
currentPolicyList={props.currentPolicyList}
setCurrentPolicyList={props.setCurrentPolicyList}
currentFlow={props.currentFlow}
target={props.target}
verb={props.verb}
allPolicies={props.allPolicies}
isAPILevelPolicy={props.isAPILevelPolicy}
listOriginatedFromCommonPolicies={props.listOriginatedFromCommonPolicies}
isApiRevision={props.isApiRevision}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we remove the prop from these instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relates to #809 (comment)

<FormattedMessage
id='Apis.Details.Policies.PoliciesExpansion.request.flow.title'
defaultMessage='Request Flow'
const PoliciesExpansionShared: FC<PoliciesExpansionSharedProps> = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Destruct props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relates to #809 (comment)

// Combine the two using a union type
type PolicyDropzoneSharedProps = PolicyDropzoneWithCommonProps | PolicyDropzoneWithoutCommonProps;

const PolicyDropzoneShared: FC<PolicyDropzoneSharedProps> = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Destruct props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relates to #809 (comment)

Copy link

sonarqubecloud bot commented Jan 3, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
37.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

2 participants