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

Chore: add Ajv JSON Schema to api/v1 #25601

Merged
merged 24 commits into from
Jun 3, 2022
Merged

Conversation

felipe-rod123
Copy link
Contributor

Proposed changes (including videos or screenshots)

This pull request adds Ajv JSON Schema validation to apps/meteor/app/api/server/v1/ and packages/rest-typings/src/v1/, where needed.

Issue(s)

Steps to test or reproduce

Further comments

@felipe-rod123 felipe-rod123 requested a review from a team as a code owner May 23, 2022 12:16
Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

put some Prop at the end of types and functions assertions

@felipe-rod123 felipe-rod123 requested a review from a team as a code owner May 30, 2022 11:42
@felipe-rod123 felipe-rod123 force-pushed the chore/add-json-schema-to-api/v1 branch from 2b778ef to 8f93798 Compare May 30, 2022 11:44
@ggazzo ggazzo force-pushed the chore/add-json-schema-to-api/v1 branch from 8378b50 to 830f53a Compare June 1, 2022 13:37
@pierre-lehnen-rc pierre-lehnen-rc self-requested a review June 3, 2022 16:13
packages/rest-typings/src/index.ts Outdated Show resolved Hide resolved
@@ -8,72 +8,536 @@ import type {
IVoipExtensionWithAgentInfo,
IManagementServerConnectionStatus,
IRegistrationInfo,
VoipClientEvents,
// VoipClientEvents,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// VoipClientEvents,

.yarnrc.yml Outdated Show resolved Hide resolved
@@ -28,7 +28,7 @@
"obj:dev": "TEST_MODE=true yarn dev",
"stylelint": "stylelint \"app/**/*.css\" \"client/**/*.css\" \"app/**/*.less\" \"client/**/*.less\" \"ee/**/*.less\"",
"stylelint:fix": "stylelint --fix \"app/**/*.css\" \"client/**/*.css\" \"app/**/*.less\" \"client/**/*.less\" \"ee/**/*.less\"",
"typecheck": "cross-env NODE_OPTIONS=\"--max-old-space-size=4092\" tsc --noEmit --skipLibCheck",
"typecheck": "cross-env NODE_OPTIONS=\"--max-old-space-size=8184\" tsc --noEmit --skipLibCheck",
Copy link
Contributor

Choose a reason for hiding this comment

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

😭

type: 'object',
properties: {
platform: {
type: 'string',
Copy link
Contributor

Choose a reason for hiding this comment

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

Since bannerPlatform is an enum, maybe you can use the enum key here?

Suggested change
type: 'string',
enum: ['1', '2'],

@@ -1,76 +1,462 @@
import type { IMessage, IRoom, ReadReceipt } from '@rocket.chat/core-typings';
import Ajv from 'ajv';
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a bit picky, but would be great to have a separate file for the AJV schemas, otherwise these definition files would be really huge (and a bit messy, depending on who sees it) 👀
(Was thinking on moving these defs to the end of the file, but there's an eslint rule we use to prevent that i think)

},
resend: {
type: 'string',
nullable: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this prop can be in optionalProperties key? 👀

additionalProperties: false,
};

export const isLivechatDepartmentDepartmentIdAgentsPOSTProps = ajv.compile<LivechatDepartmentDepartmentIdAgentsPOST>(
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of types would become more common, would be good to define an "standard" for it, like GETLivechatDepartmentIdAgents and POSTLivechatDepartmentIdAgents, prefixing the method to all typedefs

nullable: true,
},
},
required: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe required can be omitted when no prop is required

packages/rest-typings/src/v1/users.ts Show resolved Hide resolved
@ggazzo ggazzo merged commit 0b1073e into develop Jun 3, 2022
@ggazzo ggazzo deleted the chore/add-json-schema-to-api/v1 branch June 3, 2022 20:57
gabriellsh added a commit that referenced this pull request Jun 6, 2022
…message-body

* 'develop' of github.com:RocketChat/Rocket.Chat:
  [IMPROVE] Refactor + unit tests for federation-v2 (#25680)
  [FIX] user status Offline misnamed as Invisible in Custom Status edit dropdown menu (#24796)
  Chore: Messages raw model rewrite to ts (#25761)
  Chore: migrate katex to ts (#25501)
  Chore: AutoTranslate contextualBar rewrite  (#25751)
  Chore: Replace AnnouncementModal in favor of GenericModal (#25752)
  Chore: Keyboard shortcuts contextualBar rewrite (#25753)
  Chore: Prune Messages contextualBar rewrite (#25757)
  Chore: add Ajv JSON Schema to api/v1 (#25601)
  Update package.json (#25755)
  Update CODEOWNERS
  Chore: remove duplicated NotFoundPage.js (#25749)
gabriellsh added a commit that referenced this pull request Jun 6, 2022
* origin/develop: (45 commits)
  [FIX] Thread Message Preview (#25709)
  [FIX] Bump meteor-node-stubs to version 1.2.3 (#25669)
  [IMPROVE] Refactor + unit tests for federation-v2 (#25680)
  [FIX] user status Offline misnamed as Invisible in Custom Status edit dropdown menu (#24796)
  Chore: Messages raw model rewrite to ts (#25761)
  Chore: migrate katex to ts (#25501)
  Chore: AutoTranslate contextualBar rewrite  (#25751)
  Chore: Replace AnnouncementModal in favor of GenericModal (#25752)
  Chore: Keyboard shortcuts contextualBar rewrite (#25753)
  Chore: Prune Messages contextualBar rewrite (#25757)
  Chore: add Ajv JSON Schema to api/v1 (#25601)
  Update package.json (#25755)
  Update CODEOWNERS
  Chore: remove duplicated NotFoundPage.js (#25749)
  Chore: command's endpoints (#25630)
  Chore: Fix incorrect checksum for agenda package (cause of breaking develop builds) (#25741)
  Chore: Remove duplicate checksumBehavior key from yarn file (#25730)
  [FIX] Custom emoji reaction size (#25393)
  Chore: Test for department screen (#25696)
  Chore: Taking out Blaze from routes with `MainLayout`  (#25697)
  ...
gabriellsh added a commit that referenced this pull request Jun 6, 2022
* origin/develop: (26 commits)
  [FIX] Thread Message Preview (#25709)
  [FIX] Bump meteor-node-stubs to version 1.2.3 (#25669)
  [IMPROVE] Refactor + unit tests for federation-v2 (#25680)
  [FIX] user status Offline misnamed as Invisible in Custom Status edit dropdown menu (#24796)
  Chore: Messages raw model rewrite to ts (#25761)
  Chore: migrate katex to ts (#25501)
  Chore: AutoTranslate contextualBar rewrite  (#25751)
  Chore: Replace AnnouncementModal in favor of GenericModal (#25752)
  Chore: Keyboard shortcuts contextualBar rewrite (#25753)
  Chore: Prune Messages contextualBar rewrite (#25757)
  Chore: add Ajv JSON Schema to api/v1 (#25601)
  Update package.json (#25755)
  Update CODEOWNERS
  Chore: remove duplicated NotFoundPage.js (#25749)
  Chore: command's endpoints (#25630)
  Chore: Fix incorrect checksum for agenda package (cause of breaking develop builds) (#25741)
  Chore: Remove duplicate checksumBehavior key from yarn file (#25730)
  [FIX] Custom emoji reaction size (#25393)
  Chore: Test for department screen (#25696)
  Chore: Taking out Blaze from routes with `MainLayout`  (#25697)
  ...
@murtaza98 murtaza98 mentioned this pull request Jul 21, 2022
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