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

Only fetch bookmarks when the license allows to use them #8616

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

larkox
Copy link
Contributor

@larkox larkox commented Feb 19, 2025

Summary

On an unlicensed server, on development, we show errors because the bookmarks functionality is not available without a license.

To avoid these errors, we just check the license before fetching the bookmarks (and we do one less unneeded request).

Ticket Link

NONE

Release Note

NONE

@larkox larkox added the 2: Dev Review Requires review by a core commiter label Feb 19, 2025
@larkox larkox requested review from a team and hanzei and removed request for a team February 19, 2025 15:49
@larkox larkox requested a review from enahum February 19, 2025 15:50
Copy link

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Code change looks good, but CI fails. Can you please take a look?

@larkox
Copy link
Contributor Author

larkox commented Feb 20, 2025

/update-branch

@larkox larkox requested a review from hanzei February 24, 2025 15:10
@larkox larkox added the 3: QA Review Requires review by a QA tester label Feb 24, 2025
@larkox
Copy link
Contributor Author

larkox commented Feb 24, 2025

@yasserfaraazkhan The tests for this is just making sure the bookmarks are working as they should, changes in the license are properly handled, and that on unlicensed servers you don't get in the log errors on every channel switch about not having a license for bookmarks.

Copy link
Contributor

@rahimrahman rahimrahman left a comment

Choose a reason for hiding this comment

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

Wondering about the IsLicense thing.

if (!bookmarksEnabled) {
const license = await getLicense(database);

if (!bookmarksEnabled || license?.IsLicensed !== 'true') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why

IsLicensed !== `true`

means that we should not enabled bookmark? Perhaps an explanation via comment is necessary here?

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 moved around the variables to clarify the condition.

IsLicensed !== 'true' means the server is not licensed. And you need a license to use channel bookmarks.

@yasserfaraazkhan yasserfaraazkhan added Build Apps for PR Build the mobile app for iOS and Android to test E2E iOS tests for PR Run iOS E2E Detox tests labels Feb 25, 2025
@github-actions github-actions bot removed the E2E iOS tests for PR Run iOS E2E Detox tests label Feb 25, 2025
@yasserfaraazkhan
Copy link
Contributor

95 % passed just 11 failed

@larkox
Copy link
Contributor Author

larkox commented Feb 25, 2025

@yasserfaraazkhan Doesn't look like any of those e2e test are related to this PR. Is it possible they are failing on master?

@yasserfaraazkhan
Copy link
Contributor

yasserfaraazkhan commented Feb 25, 2025

ohh no @larkox

sorry I didn't give context 😀
we used to have 80% pass rate, but I was happy to see few fixes are working and now we around 95

I'm manually testing failed ones and some more tests as you suggested. Will update about the PR.

Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan left a comment

Choose a reason for hiding this comment

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

the channel bookmark can be added when server has license. we see error when there's not license.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester Build Apps for PR Build the mobile app for iOS and Android to test release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants