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

PER-9255 Clear date on backend #150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iulianvsp
Copy link
Contributor

No description provided.

@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch 2 times, most recently from db874d9 to 2a9dae3 Compare November 28, 2024 13:05
packages/api/src/folder/helper.ts Outdated Show resolved Hide resolved
packages/api/src/folder/service.ts Outdated Show resolved Hide resolved
packages/api/src/folder/models.ts Outdated Show resolved Hide resolved
packages/api/src/folder/validators.ts Outdated Show resolved Hide resolved
packages/api/src/folder/helper.ts Outdated Show resolved Hide resolved
packages/api/src/folder/controller.test.ts Outdated Show resolved Hide resolved
packages/api/src/folder/controller.test.ts Show resolved Hide resolved
@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch from 2a9dae3 to 5a5f22b Compare December 3, 2024 12:15
packages/api/docs/present/paths/folders.yaml Outdated Show resolved Hide resolved
packages/api/src/folder/models.ts Outdated Show resolved Hide resolved
packages/api/src/folder/service.ts Outdated Show resolved Hide resolved
packages/api/src/folder/validators.ts Outdated Show resolved Hide resolved
packages/api/src/folder/service.ts Show resolved Hide resolved
@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch 2 times, most recently from e631148 to c827e36 Compare December 4, 2024 15:19
packages/api/docs/present/paths/folders.yaml Show resolved Hide resolved
Comment on lines 138 to 145
const originalBehavior = db.sql.bind(db);
jest.spyOn(db, "sql").mockImplementation((name, ...params) => {
if (name === 'folder.queries.get_folder_by_id') {
throw testError;
}

return originalBehavior(name, params)
});
Copy link
Member

Choose a reason for hiding this comment

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

mockImplementationOnce() is what I usually use for these cases where I need the first DB call to succeed an a later one to fail:

test("should log error and return 500 if check for invited emails fails", async () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that, but I'm seeing some issues with this approach.

  1. I would like to mock just one sql call, I want the .sql call to actually reach the database because I want to check if the update actually happened in the DB. I did a .sql call inside the test to check that, but it will just return my mocked response.

  2. This is highly dependent on the query order sequence. Adding/Removing one sql call at some point in our code may break a lot of tests and we would have to redo the mocks based on the new order of the sql queries.

Isn't there a way to mock the response based on the function params?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's a way to do this within jest itself, but I agree it would be great to have, and it looks like this library might provide it https://github.com/timkindberg/jest-when

Copy link
Member

Choose a reason for hiding this comment

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

Though I don't know if that will provide the behavior you're looking for of "return a mocked response for this set of arguments, otherwise behave as normal". I think it would be reasonable, within this test, to check that the correct update call to db.sql is made, rather than checking that the database itself is updated. Other tests ensure that the update call correctly issued will result in an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushing the changes without jest-when, but looking into how can we use it. I think it's the more sustainable option.

packages/api/src/folder/service.ts Outdated Show resolved Hide resolved
packages/api/src/folder/service.ts Outdated Show resolved Hide resolved
packages/api/src/folder/service.ts Outdated Show resolved Hide resolved
@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch from c827e36 to 5cdc15e Compare December 6, 2024 16:08
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.94%. Comparing base (35de23b) to head (f601022).
Report is 39 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
- Coverage   94.07%   93.94%   -0.13%     
==========================================
  Files          75       91      +16     
  Lines        1366     1734     +368     
  Branches      211      258      +47     
==========================================
+ Hits         1285     1629     +344     
- Misses         74       97      +23     
- Partials        7        8       +1     

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

});
const accountArchive = accountArchiveResult.rows[0];
if (!accountArchive) {
// ??? How do I make sure I get the correct folder link id?
Copy link
Member

Choose a reason for hiding this comment

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

You are free to assume that folders will only have one non-deleted folder link. It hasn't been possible to create a folder or record with multiple folder links since ~2020. There are a relatively small number of folders and records with multiple folder links in the database, but we've been treating these as known broken data for as long as I've been at Permanent.

@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch 4 times, most recently from cbd01ed to c5bbbc8 Compare December 9, 2024 15:40
"Failed to retrieve account archive"
);
});
const accountArchive = accountArchiveResult.rows[0];
Copy link
Member

Choose a reason for hiding this comment

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

If the permissions come by way of an account_archive record, we need to check that it has edit-level permissions. Note that the Manager role exists on archive memberships, but does not exist on shares (and therefore does not exist on access records)

packages/api/src/folder/controller.test.ts Show resolved Hide resolved
@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch from c5bbbc8 to 4dd56b7 Compare December 10, 2024 10:31
@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch from 4dd56b7 to 867b30d Compare December 11, 2024 11:56
Copy link
Member

@liam-lloyd liam-lloyd left a comment

Choose a reason for hiding this comment

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

Is the empty /access/index.ts file necessary?

packages/api/src/folder/access.ts Outdated Show resolved Hide resolved
packages/api/src/folder/service.ts Outdated Show resolved Hide resolved
@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch 2 times, most recently from 2bfb666 to 904eed6 Compare December 12, 2024 14:58
"Current account archive not found"
);
}
return currentAccountArchiveResult.rows[0];
Copy link
Member

Choose a reason for hiding this comment

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

The query here could be returning multiple rows, and all of them matter to us, since we want to check whether the folder is shared with any archive the caller is a member of.

canEdit = true;
}
if (!canEdit) {
const currentAccountArchive = await accountService.getCurrentAccountArchive(
Copy link
Member

Choose a reason for hiding this comment

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

Remember that we also need to check the access role on the account_archive for the archive that received a share; if the user only has viewer permissions on the archive they shouldn't be able to edit a share with that archive regardless of the permissions of the share itself.

@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch 4 times, most recently from d91fa38 to f601022 Compare December 16, 2024 11:52
@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch 2 times, most recently from 7e91cf3 to d2bb149 Compare December 16, 2024 13:15
@slifty slifty requested a review from liam-lloyd December 16, 2024 17:11
emailFromAuthToken
);
const editMemberships = await Promise.all(
archiveMemberships.map(async (archiveMembership) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think this code will end up simpler if you use filter here instead of map (assuming Promise.all works with filter; if not this is fine).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but no, filter does not work with an async callback

Copy link
Member

@liam-lloyd liam-lloyd left a comment

Choose a reason for hiding this comment

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

Functionally, this looks good and is working. There's one hole in the test coverage, and I realized that some cases where this returns 403s should actually return 404s.

);

jest
.spyOn(folderAccess, "getAccessByFolder")
Copy link
Member

Choose a reason for hiding this comment

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

We should either not mock this, or add unit tests for access.ts, because it is currently not covered by tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was able to not mock it in some tests. Others still need mocking to to cover other scenarios unavalilable with the curent fixtures. I would say changing fixtures to not mock this at all will be a bit difficult.

How can we see the test code coverage?

Copy link
Member

Choose a reason for hiding this comment

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

You can add the --coverage flag to the test command in packages/api/package.json. The cases we're missing right now are the case where this is an access entry for the folder, and the case where the database call fails.

if (editMemberships.find((membership) => membership !== null)) {
canEdit = true;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I realized in testing that there should be two possible error messages here: the existing 403 if the user is a Viewer or Contributor to the archive where the folder lives, or if they have access to a Viewer or Contributor share of the file, and a 404 in any other case where they can't access it (that is to say, if the user isn't allowed to see the file, we shouldn't confirm for them that it exists).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, updated the code.

Copy link
Member

Choose a reason for hiding this comment

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

I think this covers every case but one - if the caller has view permissions on an archive that has an editable share of the folder, they should get a 403 instead of a 404.

@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch from d2bb149 to 452f810 Compare December 18, 2024 12:49
@iulianvsp iulianvsp force-pushed the PER-9255-clear-date-on-backend branch from 452f810 to b79c769 Compare December 19, 2024 14:13
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.

None yet

2 participants