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

Add methods for the team model #1520

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Add methods for the team model #1520

merged 2 commits into from
Jan 21, 2025

Conversation

JSReds
Copy link
Contributor

@JSReds JSReds commented Jan 12, 2025

Change-type: minor

Resolves: #
HQ:
See:
Depends-on:
Change-type: major|minor|patch


Contributor checklist
  • Includes tests
  • Includes typings
  • Includes updated documentation
  • Includes updated build output

@JSReds JSReds self-assigned this Jan 12, 2025
@JSReds JSReds force-pushed the add-team-model branch 2 times, most recently from adba519 to a8a491f Compare January 12, 2025 10:47
@JSReds JSReds marked this pull request as draft January 12, 2025 16:05
@JSReds
Copy link
Contributor Author

JSReds commented Jan 12, 2025

TODO:

Add getAll and assignToFleet methods

src/models/team.ts Outdated Show resolved Hide resolved
src/models/team.ts Outdated Show resolved Hide resolved
src/models/team.ts Outdated Show resolved Hide resolved
src/models/team.ts Outdated Show resolved Hide resolved
organizationSlugOrId: string | number,
options: BalenaSdk.PineOptions<BalenaSdk.Team> = {},
): Promise<BalenaSdk.Team[]> {
return pine.get({
Copy link
Member

Choose a reason for hiding this comment

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

  • Mark it as async
  • then add a throw in case organizationSlugOrId is unexpected

Copy link
Contributor Author

@JSReds JSReds Jan 20, 2025

Choose a reason for hiding this comment

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

do you mean by adding a pine.get(resource: 'organization', ...) or by checking if the argument is of type string | number ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with either :) IE

  • Checking for number/string and throwing invalid parameter error
  • doing just a sdk.modelsorganization.get()

src/models/team.ts Outdated Show resolved Hide resolved
src/models/team.ts Outdated Show resolved Hide resolved
src/models/team.ts Outdated Show resolved Hide resolved
src/models/team.ts Outdated Show resolved Hide resolved
src/models/team.ts Outdated Show resolved Hide resolved
@JSReds JSReds force-pushed the add-team-model branch 2 times, most recently from 5d538d1 to 070952a Compare January 20, 2025 18:14
) {
const { pine, sdkInstance } = deps;

const getRoleId = async (roleName: BalenaSdk.ApplicationMembershipRoles) => {
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 method is repeated in multiple files with different implementations, is that fine if I move it to /utils/index.ts to reuse it @thgreasi ?

teamId: number,
options: BalenaSdk.PineOptions<BalenaSdk.TeamApplicationAccess> = {},
): Promise<BalenaSdk.TeamApplicationAccess[]> {
return sdkInstance.pine.get({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we check that the teamId exist ?

teamId: number,
applicationIdOrSlug: number | string,
roleName: BalenaSdk.ApplicationMembershipRoles,
): Promise<BalenaSdk.TeamApplicationAccess> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the API already throws an error when the teamId doesn't exist, but it returns a generic internal server error. I was trying to avoid the additional request 🤔 but what do you think? Should we throw a more specific error ?

* });
*/
const update = async function (
teamApplicationAccessId: string | number,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the internal server error: the same issue applies here for teamApplicationAccessId

Comment on lines +143 to +146
applicationIdOrSlug: number | string,
roleName: BalenaSdk.ApplicationMembershipRoles,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure, but do you think it's worth updating this method to accept one of the following options?

For example:

applicationIdOrSlug: number | string | number[] | string[]  
roleName: BalenaSdk.ApplicationMembershipRoles | BalenaSdk.ApplicationMembershipRoles[]  

Or alternatively:

applicationSlugOrIdAndRole: { idOrSlug: number | string, roleName: BalenaSdk.ApplicationMembershipRoles }[]  

(just consider the idea not names that I gave as an example)

And then wrapping it in a batch function from utils. What do you think?

@JSReds JSReds requested a review from thgreasi January 20, 2025 18:31
@JSReds JSReds force-pushed the add-team-model branch 4 times, most recently from 40c47ed to cbebf6f Compare January 21, 2025 16:35
it('should be rejected if the team does not exists', function () {
const promise = balena.models.team.rename(
999999,
`new_rename_${TEST_TEAM_NAME}`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`new_rename_${TEST_TEAM_NAME}`,
`${TEST_TEAM_NAME}_new_rename`,

Comment on lines 47 to 64
it('should be able to create a new team, using organization id', async function () {
const teamName = `${TEST_TEAM_NAME}_${Date.now()}`;
const team = await balena.models.team.create(
ctx.initialOrganization.id,
teamName,
);
ctx.newTeam1 = team;
expect(team).to.have.property('name', teamName);
});
it('should be able to create a new team, using organization handle', async function () {
const teamName = `${TEST_TEAM_NAME}_${Date.now()}_2`;
const team = await balena.models.team.create(
ctx.initialOrganization.handle,
teamName,
);
ctx.newTeam2 = team;
expect(team).to.have.property('name', teamName);
});
Copy link
Member

@thgreasi thgreasi Jan 21, 2025

Choose a reason for hiding this comment

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

Suggested change
it('should be able to create a new team, using organization id', async function () {
const teamName = `${TEST_TEAM_NAME}_${Date.now()}`;
const team = await balena.models.team.create(
ctx.initialOrganization.id,
teamName,
);
ctx.newTeam1 = team;
expect(team).to.have.property('name', teamName);
});
it('should be able to create a new team, using organization handle', async function () {
const teamName = `${TEST_TEAM_NAME}_${Date.now()}_2`;
const team = await balena.models.team.create(
ctx.initialOrganization.handle,
teamName,
);
ctx.newTeam2 = team;
expect(team).to.have.property('name', teamName);
});
for (const propName of organizationRetrievalFields) {
it(`should be able to create a new team, using organization ${propName}`, async function () {
const teamName = `${TEST_TEAM_NAME}_${Date.now()}_${propName}`;
const team = await balena.models.team.create(
ctx.initialOrganization[propName],
teamName,
);
ctx.newTeam1 = team;
expect(team).to.have.property('name', teamName);
});
}

@JSReds JSReds marked this pull request as ready for review January 21, 2025 18:45
@JSReds JSReds merged commit ba9037f into master Jan 21, 2025
54 checks passed
@JSReds JSReds deleted the add-team-model branch January 21, 2025 18:46
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