Issue 5326 - Add view support for comments #5383

Bogdan-Crn · 2025-02-03T15:08:40Z

This fixes #5326

Description

This PR adds the capability of users to add a viewpoint to comments.
The viewpoint is not mandatory and is similar to the view property type, however it does not accept screenshots since you can add an image.
Added check to ensure the ticket allows comments - if not it will throw a 400 Invalid Arguments
On update comment path added middleware to ensure the comment updated exists.

Test cases

Updated unit and e2e tests to reflect the changes.

…nto ISSUE_5326

ChristosTsiotsias · 2025-02-14T15:59:08Z

I think updateComment and deleteComment methods should also be updated to unset view if its not provided and also include it in the event

ChristosTsiotsias · 2025-02-14T15:14:03Z

backend/src/v5/schemas/tickets/tickets.comments.js

@@ -40,7 +42,9 @@ const generateCommentSchema = (existingComment, isImport = false) => {
 					.test('Image ref test', 'One or more image refs do not correspond to a current comment image ref',
 						(value, { originalValue }) => !isUUIDString(originalValue)
 								|| acceptableRefs.includes(originalValue)),
-		) };
+		),
+		views: Validators.propTypesToValidator(propTypes.VIEW, !isNewComment, true, true),


views implies that there may be more than one (as it happens with images) I would probably name it view or viewpoint

could you make sure you update swagger in backend\src\v5\routes\teamspaces\projects\models\common\tickets.js ?

backend/src/v5/schemas/tickets/validators.js

ChristosTsiotsias · 2025-02-14T15:28:35Z

backend/tests/v5/e2e/routes/teamspaces/projects/models/common/tickets.comments.test.js

@@ -252,7 +252,7 @@ const testGetCommentsList = () => {
 const testCreateComment = () => {
 	describe('Create comment', () => {
 		const { users, teamspace, project, con, fed } = generateBasicData();
-		const template = ServiceHelper.generateTemplate();
+		const template = ServiceHelper.generateTemplate(false, false, { comments: true });


we could add a test to make sure we cant create comments on tickets that dont allow it (good catch btw)

agreed. this comment is unresolved, is it still outstanding?

ChristosTsiotsias · 2025-02-14T15:31:19Z

backend/tests/v5/unit/schemas/tickets/tickets.comments.test.js

+		},
+		clippingPlanes: [],
+	};
+	const invalidViewComment = {


you can just do { ... viewComment, screenshot: [existingRef2], }

.../middleware/dataConverter/inputs/teamspaces/projects/models/commons/tickets.comments.test.js

backend/src/v5/routes/teamspaces/projects/models/common/tickets.comments.js

ChristosTsiotsias

looks good, just a couple of comments that werent addressed

carmenfan

Just tagging this here as it's the most relevant place, but it's not actually a change for this file.

You need to update the processor to process the groups within the view. Take a look at how the ticket side handling groups in views, we have to save them into groups separated to the comment (otherwise we risk overloading the bson), and a reference to the group is stored in the view instead.

carmenfan · 2025-04-07T09:21:50Z

backend/src/v5/routes/teamspaces/projects/models/common/tickets.comments.js

@@ -200,6 +200,204 @@ const establishRoutes = (isFed) => {
 	 *                     format: uuid
 	 *                     description: The Id of the comment image
 	 *                     example: ef0855b6-4cc7-4be1-b2d6-c032dce7806a
+	 *                 view:
+	 *                   description: Spacial coordinates for the comment


that's not an accurate description, as that would probably better describe a pin in 3D Space

It's more like storing the state of the viewer representing what the commenter was seeing

carmenfan · 2025-04-07T09:23:00Z

backend/src/v5/routes/teamspaces/projects/models/common/tickets.comments.js

+	 *                   type: object
+	 *                   properties:
+	 *                     camera:
+	 *                       description: Details about the position and type of view


also would reword... this describe the 3D camera's position, orientation and projection

carmenfan · 2025-04-07T09:24:21Z

backend/src/v5/routes/teamspaces/projects/models/common/tickets.comments.js

+	 *                       properties:
+	 *                         position:
+	 *                           type: array
+	 *                           description: Contains the X, Y, Z coordinates


Anyone who's doing this via API should know waht a camera is in 3D graphics, anyone who don't would be pretty clueless as to what you're describing here.

So either we can skip these details, or we need to be more speific.

position is the coordinates of the camera and up/forward are the directional vectors.

type can only be orthographic or perspectiive.

carmenfan · 2025-04-07T09:24:45Z

backend/src/v5/routes/teamspaces/projects/models/common/tickets.comments.js

+	 *                           type: string
+	 *                           description: The type of camera view
+	 *                     clippingPlanes:
+	 *                       description: Some text


Some text is probably a bit too unclear 😆

carmenfan · 2025-04-07T09:35:12Z

backend/src/v5/routes/teamspaces/projects/models/common/tickets.comments.js

+	 *                                           type: array
+	 *                                           items:
+	 *                                             type: string
+	 *                         transformed:


remove this one

It's not something we want to encourage users to actively use.

carmenfan · 2025-04-07T09:37:42Z

backend/src/v5/routes/teamspaces/projects/models/common/tickets.comments.js

@@ -337,6 +535,204 @@ const establishRoutes = (isFed) => {
 	 *                       format: uuid
 	 *                       description: The Id of the comment image
 	 *                       example: ef0855b6-4cc7-4be1-b2d6-c032dce7806a
+	 *                   view:


consider the amount of copy and paste it's probably better to move it to the schema file and reference it https://github.com/3drepo/3drepo.io/blob/master/backend/src/v5/services/swagger/swagger.schemas.js

carmenfan · 2025-04-07T09:38:23Z

backend/src/v5/routes/teamspaces/projects/models/common/tickets.js

@@ -428,6 +428,204 @@ const establishRoutes = (isFed) => {
 	 *                               items:
 	 *                                 type: string
 	 *                                 description: Image in a Base64 format or an ID of an image currently used in the comment
+	 *                             view:


similar here

carmenfan · 2025-04-07T09:38:45Z

backend/src/v5/routes/teamspaces/projects/models/common/tickets.js

@@ -769,6 +967,204 @@ const establishRoutes = (isFed) => {
 	 *                               items:
 	 *                                 type: string
 	 *                                 description: Image in a Base64 format or an ID of an image currently used in the comment
+	 *                             view:


carmenfan · 2025-04-07T10:16:37Z

backend/src/v5/schemas/tickets/validators.js

 		state,
 		camera: !isUpdate && required ? camera.required() : camera,
 		clippingPlanes,
 	}).default(undefined);

+	if (!isComment) {


Think it's clearer if we do this before we turn it into a yup object

i.e.

Suggested change

if (!isComment) {

const schema = {

//...

state,

camera: !isUpdate && required ? camera.required() : camera,

clippingPlanes,

}

if(!isComment) {

schema.screenshot = types.embeddedImage(isUpdate)

}

return imposeNullableRule(Yup.object(schema).default(undefined));

carmenfan · 2025-04-07T10:17:49Z

backend/tests/v5/e2e/routes/teamspaces/projects/models/common/tickets.comments.test.js

@@ -252,7 +252,7 @@ const testGetCommentsList = () => {
 const testCreateComment = () => {
 	describe('Create comment', () => {
 		const { users, teamspace, project, con, fed } = generateBasicData();
-		const template = ServiceHelper.generateTemplate();
+		const template = ServiceHelper.generateTemplate(false, false, { comments: true });


agreed. this comment is unresolved, is it still outstanding?

Bogdan-Crn added 8 commits January 29, 2025 17:49

Issue #5326 add validation for views in comment section

5519f89

Issue #5326 - check template if comments are supported.

4c722d9

Issue #5326 - add unit tests

1bfde00

Issue #5326 - fixing the e2e tests

09293e6

Issue #5326 - fix unit tests

f61f6c8

Issue #5326 - merge branch 'staging' of github.com:3drepo/3drepo.io i…

64defe9

…nto ISSUE_5326

Issue #5326 - update documentation

ec98b52

Issue #5326 - merge branch 'staging' of github.com:3drepo/3drepo.io i…

3f2a941

…nto ISSUE_5326

Bogdan-Crn marked this pull request as ready for review February 6, 2025 16:15

Issue #5326 - fix documentation

aa23c38

Bogdan-Crn requested a review from carmenfan February 7, 2025 16:42

carmenfan requested a review from ChristosTsiotsias February 7, 2025 16:56

carmenfan assigned ChristosTsiotsias Feb 7, 2025

ChristosTsiotsias requested changes Feb 14, 2025

View reviewed changes

Bogdan-Crn added 4 commits February 24, 2025 09:45

Issue #5326 - address the feedback

939c191

Issue #5326 - merge branch 'staging'

f07fa6b

Issue #5326 - remove the comments and fix the test

69bb315

Issue #5326 - update the documentation

733b1db

Bogdan-Crn requested a review from ChristosTsiotsias February 24, 2025 15:18

Bogdan-Crn and others added 4 commits February 27, 2025 14:44

Issue #5326 - update comment fix

0a68192

Issue #5326 - merge 'staging' into ISSUE_5326

ddafabe

Issue #5326 - update the deleteComment and the tests

5515e5f

Merge branch 'staging' into ISSUE_5326

cbf026c

ChristosTsiotsias requested changes Mar 11, 2025

View reviewed changes

Bogdan-Crn added 3 commits March 25, 2025 09:41

Issue #5326 - merge staging

5f666d0

Issue #5326 - Address the rest of the feedback (hopefully)

b16f7ac

Issue #5326 - merge staging

7b6e338

Bogdan-Crn requested a review from ChristosTsiotsias March 25, 2025 17:08

carmenfan requested changes Apr 7, 2025

View reviewed changes

carmenfan assigned carmenfan and unassigned ChristosTsiotsias Apr 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 5326 - Add view support for comments #5383

Issue 5326 - Add view support for comments #5383

Bogdan-Crn commented Feb 3, 2025

ChristosTsiotsias commented Feb 14, 2025 •

edited

Loading

ChristosTsiotsias Feb 14, 2025

ChristosTsiotsias Mar 11, 2025 •

edited

Loading

ChristosTsiotsias Feb 14, 2025

carmenfan Apr 7, 2025

ChristosTsiotsias Feb 14, 2025

ChristosTsiotsias left a comment

carmenfan left a comment

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

carmenfan Apr 7, 2025

-	if (!isComment) {
+	const schema = {
+	   //...
+	   state,
+	   camera: !isUpdate && required ? camera.required() : camera,
+	   clippingPlanes,
+	}
+	if(!isComment) {
+	   schema.screenshot = types.embeddedImage(isUpdate)
+	}
+	return imposeNullableRule(Yup.object(schema).default(undefined));

Issue 5326 - Add view support for comments #5383

Are you sure you want to change the base?

Issue 5326 - Add view support for comments #5383

Conversation

Bogdan-Crn commented Feb 3, 2025

Description

Test cases

ChristosTsiotsias commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

ChristosTsiotsias Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristosTsiotsias left a comment

Choose a reason for hiding this comment

carmenfan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristosTsiotsias commented Feb 14, 2025 •

edited

Loading

ChristosTsiotsias Mar 11, 2025 •

edited

Loading