finished validation issue number:240 #281

guodong323 · 2025-02-01T22:50:23Z

No description provided.

ddfridley

First, please install prettier in visual studio code, so that when you save the file it will be formatted to the style set for this project. It makes it easier to review changes.

Then, if you npm run test there will be many test failures. And you can npm run test dturn and it will run fewer tests and that might help.

We need to make all tests pass.

I looked into this one: npm run test app/dturn/tests/rank-statements.js
The problem in that insertStatementIds is failing with

  console.error                                                                                                                                  
    User user0 is not part of discussion testRankingDiscussion

For insertStatementId we need to allow new users. This is how users join the deliberation, by adding a statement to it.

Please fix this one, and see what else comes up. There may be things we need to discuss, I don't know.

We may have to make some changes to the tests, or we may have to reconsider the validation checks, but in the end all the tests need to pass.

Thanks.

guodong323 · 2025-02-04T18:48:04Z

OK, I will solve this problem at first.

guodong323 · 2025-02-15T23:31:08Z

Test Results

insertStatementId

Allows new users to join by submitting a statement.
Ensures statements are correctly stored in ShownStatements[0].
Returns undefined if discussionId does not exist.

getStatementIds

Retrieves statement IDs correctly for users assigned statements.
Returns undefined if userId is not part of the discussion.

putGroupings

Throws an error if a user tries to group statements they have not seen.

rankMostImportant

Prevents ranking of statements not assigned to the user.
Successfully updates the ranking for statements that were shown.
Ensures rank values are either 0 or 1.

Here are some test results pictures

ddfridley

This is looking good and I'm so glad all the dturn tests are passing. Beside the comments inline, there's still a problem with the formatting. I see all these semicolons at the end of lines. We aren't using those in this project. If you install prettier into vscode it will automatically format everything on save and we won't have to worry about it. https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode (or find it for whatever editor you are using).

ddfridley · 2025-02-20T23:38:05Z

app/dturn/__tests__/rank-statements.js

    expect(statements).toBeDefined()
-    expect(statements.length).toBe(10)
+    expect(statements.length).toBeGreaterThanOrEqual(2)


statements.length must be 10 -- or Options.group_size. If it's not then lets discuss.

ddfridley · 2025-02-20T23:38:48Z

app/dturn/__tests__/rank-statements.js

-    for (let i = 0; i < 20; i++) {
-      props.push([DISCUSSION_ID, `user${i}`, `statement${i}`])
+
+    console.log("After initDiscussion:", JSON.stringify(Discussions[DISCUSSION_ID], null, 2));


please remove the console.log statements since you have it working.

ddfridley · 2025-02-20T23:40:04Z

app/dturn/dturn.js

-    shownCount: 0,
-    rank: 0,
+  if (!Discussions[discussionId].Uitems[userId]) {
+    console.log(`User ${userId} is new, adding to discussion ${discussionId}`);


please remove console.log statements.

ddfridley · 2025-02-20T23:43:48Z

app/dturn/dturn.js

+    return undefined;
+  }
+
+  // 确保 `statementId` 存在于 `ShownStatements`


comments are good, but please make them in english.

Thank you, I will pay attention to this

ddfridley · 2025-02-20T23:52:33Z

app/dturn/dturn.js

-      dis.ShownStatements[round][cutoff].rank > dis.min_rank ? dis.ShownStatements[round][cutoff].rank : dis.min_rank
+    const cutoff = Math.ceil(dis.ShownStatements[round].length / dis.group_size);
+    const minRank = Math.max(dis.ShownStatements[round][cutoff].rank, dis.min_rank);
+
    if (delta > 0) {
      if (sitem.rank >= minRank) {
        if (!dis.ShownStatements[round + 1].some(s => s.statementId === sitem.statementId)) {


please put back the 2 lines of comments

ddfridley · 2025-02-21T00:03:21Z

app/dturn/tests/dturn.js

-// Tests
-test('Succeed if all options are valid.', async () => {
-  initDiscussion(DISCUSSION_ID, { group_size: 15 })
-})
-
-test('Fail if all options are not valid.', async () => {
-  const func = async () => {
-    await initDiscussion(DISCUSSION_ID, { a_nonexistent_option: 12345 })
-  }
-  expect(func()).rejects.toThrow(nonexistentOptionErrorRegex)
-})
-
-test('Fail if at least 1 option is not valid.', async () => {
-  const func = async () => {
-    await initDiscussion(DISCUSSION_ID, { group_size: 10, a_nonexistent_option: 12345 })
-  }
-  expect(func()).rejects.toThrow(nonexistentOptionErrorRegex)
-})


These tests seem to be missing?

Yes, these cases have already been covered in other test cases.

ddfridley · 2025-02-21T00:07:31Z

app/dturn/tests/dturn.js

+    Object.keys(Discussions).forEach(key => delete Discussions[key]);
+
+    // Initialize a test discussion with required properties
+    Discussions.testDiscussion = {


We should try to use initDiscussion rather than directly manipulating it.

guodong323 · 2025-02-23T21:45:13Z

This is looking good and I'm so glad all the dturn tests are passing. Beside the comments inline, there's still a problem with the formatting. I see all these semicolons at the end of lines. We aren't using those in this project. If you install prettier into vscode it will automatically format everything on save and we won't have to worry about it. https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode (or find it for whatever editor you are using).

Hi David. I already fixed these format problem. I downloaded prettier and make the format is the same with out codebase.

ddfridley

This is going really well, and I see some good changes to dturn.js. I know it's hard working in other peoples code, and this code is especially complicated.

While all of the dturn tests pass, it turns out there are a few others when I do npm run test that don't pass.

npm run test app/socket-apis/__tests__/complete-round.js app/socket-apis/__tests__/post-point-groupings.js app/socket-apis/__tests__/get-points-for-round.js

In one case I see getStatementIds is returning [] instead of undefined, so dturn needs to be fixed for that.
For some of the others, it may be that the test needs to be fixed. Can you look at them, and see what you can do, but if it's gets too deep then I can take a turn figuring them out.

Also, I pushed some changes, so you will need to git pull in this branch. I put back some comments that were taken out, that are still valuable. What I learned is that comments about why a piece of code is there are the most valuable. I also changed the other throw's .to cosole.errors. When code is being used in api calls, throws leave the api call incomplete. I know I'm the one that put the throws there. But since I was in there it was time to get rid of them.

Have a look through this commit to see what I changed: deeb271

ddfridley · 2025-02-26T06:50:44Z

app/dturn/__tests__/rank-statements.js

    expect(statements).toBeDefined()
    expect(statements.length).toBe(10)

+    console.log('Retrieved statements:', statements)


okay, now that this code is working isn't it time to take the console.log statements out.

ddfridley · 2025-02-26T06:55:13Z

package.json

@@ -71,6 +71,7 @@
    "@babel/preset-react": "^7.16.0",
    "@svgr/cli": "^8.1.0",
    "babel-loader": "^8.2.3",
+    "prettier": "3.5.2",


Was prettier required to get it working in vscode? -- are you using vs code or something else?
I don't want to have prettier as a dependency if we don't have to. But if that was required then lets do it. But can you uninstall prettier and see if it's working in your editor?
Also, if it is required it should be in optionalDependencies. You have to put it there manually in the editor. When we push to production on heroku, devDependencies are installed and build and used to build the repo. optionalDependencies are not installed in order to save space and build time.

guodong323 · 2025-03-17T17:39:47Z

This is going really well, and I see some good changes to dturn.js. I know it's hard working in other peoples code, and this code is especially complicated.

While all of the dturn tests pass, it turns out there are a few others when I do npm run test that don't pass.
npm run test app/socket-apis/__tests__/complete-round.js app/socket-apis/__tests__/post-point-groupings.js app/socket-apis/__tests__/get-points-for-round.js
In one case I see getStatementIds is returning [] instead of undefined, so dturn needs to be fixed for that. For some of the others, it may be that the test needs to be fixed. Can you look at them, and see what you can do, but if it's gets too deep then I can take a turn figuring them out.

Also, I pushed some changes, so you will need to git pull in this branch. I put back some comments that were taken out, that are still valuable. What I learned is that comments about why a piece of code is there are the most valuable. I also changed the other throw's .to cosole.errors. When code is being used in api calls, throws leave the api call incomplete. I know I'm the one that put the throws there. But since I was in there it was time to get rid of them.

Have a look through this commit to see what I changed: deeb271

passed all tests

ddfridley

Okay, I think there was a misunderstanding about how to handle the tests failing. It is the design intent that getStatementIds returns undefined rather than [] when there are no statements to work on. If undefined is returned sometimes, and [] other times, then the code has to test for if(!ids || !ids.length) where as if it just returns undefines, then the test is just if(!ids). Can you please change it the other way. And FYI - there's another test. node app/dturn/test.js which runs there a big loop of putting stuff in to a discussion, and then validating it. I don't have it in jest because it takes a long time to run.
Please look through the changes in the PR Files Changed and put back the comments that were taken out. Some of them may not be appropriate anymore, but many of them are and comments are hard to get.
please do git checkout origin/master package.json and git checkout origin/master package-lock.json. The changes to those files aren't relevant to this issue.

ddfridley · 2025-03-20T04:21:17Z

app/dturn/dturn.js

-      throw new Error(
-        `getStatments unexpected number of statments ${JSON.stringify(dis.Uitems?.[userId]?.[round], null, 2)}`
-      )
+      return sIds.length > 0 ? sIds : []


this is a long way of saying return sIds - please shorten.

ddfridley · 2025-03-20T04:28:06Z

app/dturn/dturn.js

@@ -336,7 +345,7 @@ async function getStatementIds(discussionId, round, userId) {

  await dis.updateUInfo(delta)

-  return statementIds
+  return statementIds.length > 0 ? statementIds : []


here is another case where it's the long way of saying return statementIds

finished validation issue number:240

fc11506

guodong323 requested a review from ddfridley February 1, 2025 22:50

ddfridley linked an issue Feb 3, 2025 that may be closed by this pull request

Dturn prop validation #240

Open

8 tasks

ddfridley requested changes Feb 3, 2025

View reviewed changes

fixed some tests bugs

111ed2f

ddfridley requested changes Feb 21, 2025

View reviewed changes

fixed some syntax error

f1201c9

put some comments back, no throws

deeb271

ddfridley requested changes Feb 26, 2025

View reviewed changes

fixed some other tests

3429b2d

ddfridley requested changes Mar 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

finished validation issue number:240 #281

finished validation issue number:240 #281

guodong323 commented Feb 1, 2025

ddfridley left a comment

guodong323 commented Feb 4, 2025

guodong323 commented Feb 15, 2025

ddfridley left a comment

ddfridley Feb 20, 2025

ddfridley Feb 20, 2025

ddfridley Feb 20, 2025

ddfridley Feb 20, 2025

guodong323 Feb 23, 2025

ddfridley Feb 20, 2025

ddfridley Feb 21, 2025

guodong323 Feb 23, 2025

ddfridley Feb 21, 2025

guodong323 commented Feb 23, 2025

ddfridley left a comment

ddfridley Feb 26, 2025

ddfridley Feb 26, 2025

guodong323 commented Mar 17, 2025

ddfridley left a comment

ddfridley Mar 20, 2025

ddfridley Mar 20, 2025

finished validation issue number:240 #281

Are you sure you want to change the base?

finished validation issue number:240 #281

Conversation

guodong323 commented Feb 1, 2025

ddfridley left a comment

Choose a reason for hiding this comment

guodong323 commented Feb 4, 2025

guodong323 commented Feb 15, 2025

Test Results

ddfridley 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

guodong323 commented Feb 23, 2025

ddfridley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guodong323 commented Mar 17, 2025

ddfridley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment