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

Feature/add test cd #63

Open
wants to merge 19 commits into
base: development
Choose a base branch
from
Open

Feature/add test cd #63

wants to merge 19 commits into from

Conversation

idelcano
Copy link
Contributor

📌 References

  • Issue: Coding dojo

📝 Implementation

Added some test for user-permission script (In this case we change a bit the workflow and I did a manual test also to be sure that nothing is broken).

Should ignore user if the user is in the datastore excluded users list
Should ignore user if the user has valid roles
Should fix user if the user has invalid authorities
Should throw exception if the user don't have a valid user template group and fix is disabled
Should fix add minimal group if a user dont have any control template user group
Should push fixed usergroup if the user has no template usergroups
Should apply minimal control template group if a user dont have any control template user group

We could continue adding test due has a lot of corner case to test.

📹 Screenshots/Screen capture

🔥 Notes to the tester

yarn start usermonitoring run-permissions-fixer --config-file config.json

@idelcano idelcano requested a review from xurxodev November 13, 2024 18:58
Copy link

@xurxodev xurxodev left a comment

Choose a reason for hiding this comment

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

thanks @idelcano

I have found some issues about quality of the code to review. I've added comments in the code

clonedInvalidTemplateAuthorities = copyObject(permissionFixerInvalidTemplateGroupsExtended);
});

function getMetadataConfig(): PermissionFixerMetadataConfig {

Choose a reason for hiding this comment

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

This function is not necessary and generate noise, you can use the variable direcly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All fixed

}

function getMetataConfiThrowInvalidUsergroupException(): PermissionFixerMetadataConfig {
return configThrowInvalidUsergroupException;

Choose a reason for hiding this comment

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

This function is not necessary and generate noise, you can use the variable direcly

}

function getMetataConfigWithExcludedUser(): PermissionFixerMetadataConfig {
return configWithUserExcluded;

Choose a reason for hiding this comment

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

This function is not necessary and generate noise, you can use the variable direcly

}

function getWrongMinimalUserGroupConfig(): PermissionFixerMetadataConfig {
return configWithWrongMinimalGroup;

Choose a reason for hiding this comment

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

This function is not necessary and generate noise, you can use the variable direcly

}

function getValidUser(): PermissionFixerUser {
return clonedValidUser;

Choose a reason for hiding this comment

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

This function is not necessary and generate noise, you can use the variable direcly

}

function getInvalidUser(): PermissionFixerUser {
return clonedInvalidUser;

Choose a reason for hiding this comment

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

This function is not necessary and generate noise, you can use the variable direcly

}

function getUserWithoutGroup(): PermissionFixerUser {
return clonedFakeUserWithoutGroup;

Choose a reason for hiding this comment

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

This function is not necessary and generate noise, you can use the variable direcly

);
return useCase;
}
function givenBasicConfigRepository() {

Choose a reason for hiding this comment

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

There are a lot of similar functions here where the unique difference is the return when mockedRepository.get() is invoked, You can create a unique function and pass by parameter the data to return.

@idelcano idelcano requested a review from xurxodev December 30, 2024 12:29
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