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 - display all domains that reference current user #2789

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ArtjomsPorss
Copy link
Contributor

Description

Change to UI domains list for current user to include domains with indirect membership, e.g. domains that assume delegated roles.

Contribution Checklist:

  • The pull request does not introduce any breaking changes
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

ui/src/api.js Outdated
Comment on lines 38 to 44

listUserDomains(roleName) {
listUserDomains() {
return new Promise((resolve, reject) => {
fetchr
.read('domain-list')
.params({ roleName })
.read('domain-role-member')
.params({ expand: true })
.end((err, data) => {
Copy link
Contributor Author

@ArtjomsPorss ArtjomsPorss Nov 1, 2024

Choose a reason for hiding this comment

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

using different api to obtain domain details. not passing principal as it is picked up from the request context

@ArtjomsPorss ArtjomsPorss marked this pull request as ready for review November 1, 2024 15:44
@ArtjomsPorss ArtjomsPorss force-pushed the ui-display-all-domains-for-principal branch 2 times, most recently from 930d5ca to fdd4fe9 Compare November 1, 2024 16:47
@ArtjomsPorss ArtjomsPorss force-pushed the ui-display-all-domains-for-principal branch from fdd4fe9 to f0d2cee Compare November 5, 2024 17:09
Comment on lines 270 to 313


const transformDomainListResult = (domainsData) => {
let domainsToReturn = [];

// 'prefix' is a list of unique domain names for given principal
if (!domainsData || !domainsData.memberRoles || !domainsData['prefix']) {
return domainsToReturn;
}

let adminDomains = new Set();
let nonAdminDomains = domainsData['prefix'].slice(); // list of all unique domain names

// processing non-admin domains separately because user can be an admin of same domain only once
// but can be non-admin member of same domain via multiple roles/groups
domainsData.memberRoles.forEach(role => {
if (role.roleName === 'admin') {
// create admin domain object
adminDomains.add({
name: role.domainName,
adminDomain: true
});
// remove admin domain from list of non admin domains
const adminDomainIndex = nonAdminDomains.indexOf(role.domainName);
nonAdminDomains.splice(adminDomainIndex, 1);
}
});

// add admin domains
domainsToReturn.push(...adminDomains);

// create and add non admin domains
nonAdminDomains.forEach(domain => {
domainsToReturn.push({
name: domain,
adminDomain: false
})
})

// sorting domains alphabetically
domainsToReturn.sort((a, b) => a.name.localeCompare(b.name));

return domainsToReturn;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to docs thunks is the preferred location for data transformation, not the reducers

Comment on lines +54 to 56
const domainsData = await API().listUserDomains();
const domainsList = transformDomainListResult(domainsData);
dispatch(loadUserDomainList(domainsList));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

transforming data returned by fetchr

@ArtjomsPorss
Copy link
Contributor Author

ArtjomsPorss commented Nov 5, 2024

By changing API call - the returned data format was changed thus breaking the snapshot generation.
Multiple tests had to be fixed because they seem to be used by jest as a base for generating snapshots.

@ArtjomsPorss ArtjomsPorss marked this pull request as draft November 5, 2024 17:44
@ArtjomsPorss ArtjomsPorss marked this pull request as ready for review November 5, 2024 21:12
@ArtjomsPorss
Copy link
Contributor Author

js file reformatting was done on some files unrelated to pr because these files were failing github build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only formatting change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only formatting change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only formatting change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only formatting change

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.

1 participant