-
Notifications
You must be signed in to change notification settings - Fork 11
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
[WIP] 433 anonymously uploaded files should appear in trash of folder creator #536
[WIP] 433 anonymously uploaded files should appear in trash of folder creator #536
Conversation
c01d111
to
2d04040
Compare
Coverage Report
Click to view remaining coverage report
|
692e848
to
e6e6931
Compare
56bc7b2
to
107bbf6
Compare
/** Run the provided callback using the specified bearer JWT token. | ||
* //TODO: Warning: does not override calls using `this.api` have to discuss | ||
*/ | ||
async impersonateWithJWT<T>(jwt: string, cb: () => Promise<T>): Promise<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed to rollback to the old token.
For anonymous access you can crate a new instance of the UserApi in the test, and store the token permanently.
These callbacks just bring complexity
const anonymousUser = UserApi()
anonymousUser.loginWithToken()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more complicated than that, it's what I wanted to discuss. The UserAPI class has a user object that wouldn't be valid with the other JWT, it also loads workspace and anonymous user stuff which we don't need, and it inits the Api class with the user, which then loads it's jwt on its own. I can't figure out what they're supposed to be but they don't match a user nor a session one to one. This is a hacky way around all that complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This is a hacky way around all that complexity.", exactly, so please let's do not add additional.
Actually it represents a user session in the browser, and sometimes it can be with "real" user or anonymous session.
So we can refactor the constructor of the class, and make some fields optional, but until this, let's KISS
anonymousUser.jwt = token.value;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤮 that's worse ! at least my hack is clean and has no side effects (also I disagree it's complex, it's one callback that's advised), and the call is readable and descriptive. Modifying the field from outside on an object used in multiple tests....
Yes I want to refactor UserApi, and also, remove User-Api, but, we should talk first because I'm not sure about the actual responsibility scope we should aim for
783f9df
to
c5c7f70
Compare
expect(await getTrashContentIds("shared")).not.toContain(createItemResult.id); | ||
}); | ||
|
||
describe("deleting a file uploaded by an anonymous user should go to the sharers trash", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always? I thought it should depend on whare this file was uploaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I'm understanding: it goes to the owner of the shared folder, so yes it depends on where it was uploaded ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"sharers trash" was not understandable for me without ', it thought it's a misprint from "shared"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, yeah, maybe I should find a better way to write that... I want to avoid '
's just to make the CLI easier to use without escaping to filter the tests
…eted e2e_deleteDocument (#433)
c5c7f70
to
06d6db2
Compare
#433 WIP
very very strict minimum mvp