initial attempt to fix title > 255 char

[Victor-M-Giraldo]

closes #312

[dyc3]

I took a quick took, turns out I don't have unit tests for the patch endpoint already, but I do have one for the create endpoint here:

opentogethertube/server/tests/unit/api/room.spec.ts

Line 139 in 67023d5

describe("POST /room/create", () => {

since you can include a title when creating a room too, you'll need to check the title in there too

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6126213) 61.4426% compared to head (934465f) 61.6270%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master      #1189        +/-   ##
================================================
+ Coverage   61.4426%   61.6270%   +0.1843%     
================================================
  Files           115        115                
  Lines          9316       9379        +63     
  Branches       1139       1141         +2     
================================================
+ Hits           5724       5780        +56     
- Misses         3592       3599         +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Victor-M-Giraldo]

I took a quick took, turns out I don't have unit tests for the patch endpoint already, but I do have one for the create endpoint here:

opentogethertube/server/tests/unit/api/room.spec.ts

Line 139 in 67023d5

describe("POST /room/create", () => {

since you can include a title when creating a room too, you'll need to check the title in there too

Currently the change I made kind of works on my local copy? When I try to edit the title to be > 255 chars a 401 response is sent, but when you try to make any edits afterwards nothing works and the room settings cannot be loaded.

What are the differences between createRoom & generateRoom? In createRoom the title property does not exist. Also, if I use the same code I used for the patch request I get an error where the field is not recognized as a property.

I figured I would need to mess with createRoom, but that seems to not be the case.

[dyc3]

Yes, createRoom is the function that creates a room.

Making a HTTP request: POST /api/room/create calls createRoom, which creates a named room. Making a HTTP request: POST /api/room/generate calls generate, which creates a room with a randomly generated UUID name.

Looks like you'll need to add title?: string to OttApiRequestRoomCreate to get rid of that error. I did not exactly make that type comprehensive.

but when you try to make any edits afterwards nothing works and the room settings cannot be loaded.

I'll try it on my machine.

[dyc3]

Make sure all the required CI is passing before you mark it as ready to review. yarn lint will fix formatting errors and some lints automatically, yarn test will run tests. You might want to turn on format on save in vscode.

[Victor-M-Giraldo]

Make sure all the required CI is passing before you mark it as ready to review. yarn lint will fix formatting errors and some lints automatically, yarn test will run tests. You might want to turn on format on save in vscode.

Got it.

[Victor-M-Giraldo]

I did run yarn test & yarn lint, as well as turning on format on save. I'm not sure why the tests are failing, however. The changes I made seem to be working though.

[dyc3]

CI says tests pass, but there's formatting errors. You can click details on the failing job to see the logs. But tbh yarn lint should have fixed those things, so make sure it's not giving you errors.

[Victor-M-Giraldo]

It gives me 16 warnings, 0 errors. It's giving me those warnings in files I haven't touched at all yet though. Any idea why?

Also, two things.

I see that rust build failed to run and that is required to merge. Not sure why.

Additionally, when I run the test suite some test suites fail to run. Jest says it's finding 6 handles keeping it from exiting.

[dyc3]

yarn lint will modify files for formatting without emitting errors, so check if you have any uncommitted changes.

It's giving me those warnings in files I haven't touched
Don't worry about those. Haven't fixed them yet.

The rust tests are a bit flaky right now, so don't worry about it.

The jest open handle thing is normal, it doesn't affect the test results.

[Victor-M-Giraldo]

Stupid mistake. Was on the wrong branch. Should hopefully be good now.

[dyc3]

Diff looks good. Do you want to add unit tests for this? Totally optional, I'll add them if you don't.

[Victor-M-Giraldo]

Diff looks good. Do you want to add unit tests for this? Totally optional, I'll add them if you don't.

I want to try and give it a shot.

[Victor-M-Giraldo]

Added the unit test for the create endpoint. Ran yarn lint & test.

Not sure if yarn lint did anything though.

Besides that, a few questions.

In room.spec.ts where you describe the tests for the create endpoint, what do getSessionInfoSpy and validateSpy do?

What purpose do the tokens serve, are they a type of identifier?

If I am creating unit tests for the other endpoint, would I need to declare the app variable again and include the beforeAll, afterAll and afterEach again?

Also, in common/models are those files similar to mongoose schemas? Basically, are they how the database tables are set up?

[dyc3]

what do getSessionInfoSpy and validateSpy do?
Those are jest mock functions. They are mocked because calling the real versions of those functions requires redis, and I deliberately don't want to require redis to run unit tests because that would ultimately reduce test isolation.

What purpose do the tokens serve?
The auth tokens are used to keep track of sessions. Sessions keep track of what account they are logged in as, or what username they currently have.

If I am creating unit tests for the other endpoint, would I need to declare the app variable again and include the beforeAll, afterAll and afterEach again?

Inside the describe("Room API", () => { block, you don't need to define app again. It's already in scope. Inside of that block, you would need to add a new describe. Something like this:

describe("POST /api/room/:name", () => {
   beforeAll(() => {
       // create a temporary room to test against
    });

    afterAll(() => {
        // unload all rooms
    });

    it.each([
        [expectedError, requestBody],
    ])("should fail to modify room for validation errors: %s", async (error, body) => {
        // test code here
    });
});

No need to add a bunch of cases to get full coverage of the endpoint, just write enough to cover the changes in this PR.

The stuff in common/models is basically just type definitions shared across the server and client (its a poor name choice, haven't bothered to change it). Not really related to db models. Those are handled with sequelize, see server/models.

[dyc3]

Looks good. just a minor thing