Skip to content
This repository was archived by the owner on Jul 6, 2022. It is now read-only.

feat: update services to latest versions#395

Merged
karolsojko merged 3 commits intomasterfrom
update_services
Aug 26, 2021
Merged

feat: update services to latest versions#395
karolsojko merged 3 commits intomasterfrom
update_services

Conversation

@karolsojko
Copy link
Copy Markdown
Member

No description provided.

@karolsojko
Copy link
Copy Markdown
Member Author

@antsgar I need your help on this one.

I've tried to check if SNJS is working on latest auth and ssjs.

One test fails with the features service update roles observer: https://github.com/standardnotes/snjs/blob/master/packages/snjs/lib/services/features_service.ts#L45-L48

if you run only this test: https://github.com/standardnotes/snjs/blob/master/packages/snjs/test/lib/session.test.js#L105 - it passes

if you run only these 2 tests: https://github.com/standardnotes/snjs/blob/master/packages/snjs/test/lib/session.test.js#L90-L112

the second one fails. Seems like the item manager is undefined after the first test passes and second runs.

Could you have check it out?

@antsgar
Copy link
Copy Markdown
Contributor

antsgar commented Aug 25, 2021

@karolsojko sure I'll check it out!

@antsgar
Copy link
Copy Markdown
Contributor

antsgar commented Aug 25, 2021

@karolsojko is what you described happening when you run the tests locally? I can't seem to be able to reproduce it

@antsgar
Copy link
Copy Markdown
Contributor

antsgar commented Aug 25, 2021

Nevermind, I could reproduce now, will take a look 🙂

@antsgar
Copy link
Copy Markdown
Contributor

antsgar commented Aug 25, 2021

@karolsojko so the problem is the methods that call api-gateway endpoints don't await for the meta field to be processed (roles to be updated, features to be fetched and mapped). This was intentional so that the response time isn't incremented. However, in this case what's happening is the app is being deinited after the meta response from /sync has been received, but before its processing has finished. I committed a change that would fix it, but this might be bound to give us some headaches with other tests in the future.

(Note: This didn't happen before this PR because the /features endpoint isn't yet prod released in auth. Since the endpoint wasn't returning a response before, the service wasn't trying to map the features from that response.)

@karolsojko
Copy link
Copy Markdown
Member Author

Thanks @antsgar so much for the help - actually I'm realeasing auth and ssjs to prod tmrw so this was the last check before it goes out :)

@antsgar
Copy link
Copy Markdown
Contributor

antsgar commented Aug 25, 2021

@karolsojko one thing I noticed is /features is returning an empty array in tests, even though the user role returned is BASIC_USER. At least basic features should be returned I think? Perhaps we should give that a check

@karolsojko
Copy link
Copy Markdown
Member Author

@antsgar I think the reason is that the user has to not only have the role but also an active subscription corresponding to that role: https://github.com/standardnotes/auth/blob/develop/src/Domain/Feature/FeatureService.ts#L34-L38

does that make sense?

@karolsojko karolsojko changed the title feat: check if latest develop is compatible with snjs feat: update services to latest versions Aug 26, 2021
@karolsojko karolsojko merged commit 8082ebd into master Aug 26, 2021
@karolsojko karolsojko deleted the update_services branch August 26, 2021 07:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants