Skip to content

Conversation

@Bogdan-Crn
Copy link
Contributor

This fixes #5727

Description

Updated the teamspace endpoints to distinguish between internal and external endpoints.
Created an internal endpoint to allow creation of teamspaces.
Added data validation for the request.
Updated tests for validation and e2e.

Acceptance Criteria

  • as a system integrator I want to be able to create a teamspace systematically when needed
  • teamspace name doesn't conform to the string requirements (alphanumeric, no full stops, max 128 chars)
  • teamspace already exists
  • account Id is provided but does not exist
  • admin is provided but not an email

.matches(/^[a-zA-Z0-9]+$/, 'Teamspace name must be alphanumeric and contain no full stops.')
.max(128, 'Teamspace name must be at most 128 characters long.')
.required('Teamspace name is required'),
admin: Yup.string().email('Admin must be a valid email address').optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have a type for this in backend\src\v5\utils\helper\yup.js

YupHelper.types.strings.email

req.body = deleteIfUndefined(await schema.validate(req.body, { stripUnknown: true }));

try {
teamspaceExists = await getTeamspaceSetting(req.body.name, { _id: 1 });
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be in a test in name property, take this as an example

Image

if (teamspaceExists) {
throw new Error('Teamspace with this name already exists.');
}
if (req.body.admin) await getUserByEmail(req.body.admin);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed, we dont have to check that the user exists, just the format

Teamspaces.validateCreateTeamspaceData = async (req, res, next) => {
const schema = Yup.object().shape({
name: Yup.string()
.matches(/^[a-zA-Z0-9]+$/, 'Teamspace name must be alphanumeric and contain no full stops.')
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have this type in we already have a type for this in backend\src\v5\utils\helper\yup.js

YupHelper.validators.alphanumeric

* type: string
* description: The account ID the teamspace will belong to
* example: 3fa85f64-5717-4562-b3fc-2c963f66afa6
* owner:
Copy link
Contributor

Choose a reason for hiding this comment

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

admin*

FrontEggAccounts.getTeamspaceByAccount.mockResolvedValueOnce(undefined);

await Teamspaces.validateCreateTeamspaceData(req, {}, mockCB);
expect(mockCB).toHaveBeenCalledTimes(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

not.ToHaveBeenCalled()


describe.each([
['if valid name', { body: { name: 'ValidTeamspaceName' } }, true],
['if name at max length', { body: { name: 'A'.repeat(128) } }, true],
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to test exactly 128 any length and invalid length would do

describe.each([
['if valid name', { body: { name: 'ValidTeamspaceName' } }, true],
['if name at max length', { body: { name: 'A'.repeat(128) } }, true],
['if name too long', { body: { name: 'A'.repeat(129) } }, false],
Copy link
Contributor

Choose a reason for hiding this comment

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

you can call generateRandomString(129)

['if valid name', { body: { name: 'ValidTeamspaceName' } }, true],
['if name at max length', { body: { name: 'A'.repeat(128) } }, true],
['if name too long', { body: { name: 'A'.repeat(129) } }, false],
['if name with full stop', { body: { name: 'Invalid.Teamspace.Name' } }, false],
Copy link
Contributor

Choose a reason for hiding this comment

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

if name includes* full stop

agent = await SuperTest(server);
const isInternal = [true, false];

describe.each(isInternal)('Internal flag set to %s', (internalFlag) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not sure if we have to test all endpoints with different IsInternal flags or just make sure the internal-only are not available in external service @carmenfan could you check this ?

Copy link
Member

Choose a reason for hiding this comment

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

no just the ones that we have exposed should be checked on both configuration.

If we can conform to the same way we've done it with other endpoints that got merged then it'll be easier for everyone to read.

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.

4 participants