Page MenuHomePhabricator
Differential D7188

[web-db] support SQLite for staff and non-mobile browsers
ClosedPublic
Actions

Authored by kamil on Mar 27 2023, 6:21 AM.
Tags
None
Referenced Files
F1694248: D7188.diff
Thu, May 2, 10:04 PM
Unknown Object (File)
Sun, Apr 7, 8:01 AM
Unknown Object (File)
Wed, Apr 3, 8:02 AM
Unknown Object (File)
Wed, Apr 3, 8:02 AM
Unknown Object (File)
Wed, Apr 3, 8:02 AM
Unknown Object (File)
Wed, Apr 3, 8:02 AM
Unknown Object (File)
Wed, Apr 3, 8:02 AM
Unknown Object (File)
Wed, Apr 3, 8:01 AM
View All 82 Files
Subscribers
ashoat
atul

Details

Reviewers
michal
tomek
ashoat
Commits
rCOMM852d009eb408: [web-db] support SQLite for staff and non-mobile browsers
Summary

Logic based on detect-browser

Test Plan
  1. Test for web app and electron
  2. Test in browser on mobile

Diff Detail

Repository
rCOMM Comm
Branch
land-db
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil created this revision.Mar 27 2023, 6:21 AM
kamil held this revision as a draft.
Herald added subscribers: atul, ashoat. · View Herald TranscriptMar 27 2023, 6:21 AM
kamil added a child revision: D7189: [web-db] add database module provider.Mar 27 2023, 6:24 AM
Harbormaster failed remote builds in B17662: Diff 24182!Mar 27 2023, 6:37 AM
kamil updated this revision to Diff 24189.Mar 27 2023, 6:51 AM

annotate function return type

Harbormaster completed remote builds in B17669: Diff 24189.Mar 27 2023, 7:06 AM
kamil published this revision for review.Mar 27 2023, 8:15 AM
ashoat requested changes to this revision.Mar 27 2023, 4:09 PM
ashoat added inline comments.
keyserver/src/responders/website-responders.js
433 ↗(On Diff #24189)

I guess perhaps headers are case-insensitive, but elsewhere in the app we use User-Agent

Can we actually just get this from viewer.userAgent?

435 ↗(On Diff #24189)

What happens if the user logs in as a staff user, but then logs out and logs back in as a non-staff? (Or vice versa?) Seems like it can get messy... is there any way to determine this on the client instead?

444 ↗(On Diff #24189)

Can you explain in more detail why you need this to be determined on the keyserver and passed in from there? Why can't we decide this on the client?

keyserver/src/utils/validation-utils.js
182–185 ↗(On Diff #24189)

Can we use browser-detect, which we already use elsewhere in our codebase?

This revision now requires changes to proceed.Mar 27 2023, 4:09 PM
ashoat mentioned this in D7217: [keyserver] Introduce endpoint for deep linking.Mar 28 2023, 5:48 AM
kamil updated this revision to Diff 24551.Apr 3 2023, 2:23 AM

move logic to client-side

kamil added a comment.Apr 3 2023, 2:27 AM

Thanks, @ashoat. You identified a severe issue with doing it on the keyserver. Moving logic to client-side

web/database/utils/constants.js
11 ↗(On Diff #24551)

win 11 is still detected as 11 due to the old kernel version (issue).

I didn't include older windows versions as they're not officially supported by Microsoft anymore.

16–23 ↗(On Diff #24551)

based on caniuse

kamil edited the summary of this revision. (Show Details)Apr 3 2023, 2:31 AM
Harbormaster completed remote builds in B17921: Diff 24551.Apr 3 2023, 2:38 AM
kamil added inline comments.Apr 3 2023, 5:58 AM
web/database/utils/constants.js
16 ↗(On Diff #24551)

this is possible:

import { Browser } from 'detect-browser';

...

export const DB_SUPPORTED_BROWSERS: $ReadOnlyArray<Browser> = [

but to do that we need to add flow types for detect-browser

ashoat accepted this revision.Apr 3 2023, 1:57 PM
ashoat added inline comments.
web/database/utils/constants.js
11 ↗(On Diff #24551)

@kamil meant "detected as 10" here I think (based on linked issue)

web/database/utils/db-utils.js
37–38 ↗(On Diff #24551)

I would replace this with:

if (!isDev && (!currentLoggedInUserID || !isStaff(currentLoggedInUserID))) {
  return false;
}
40–43 ↗(On Diff #24551)

And then this could be:

return DB_SUPPORTED_OS.includes(browser.os)
  && DB_SUPPORTED_BROWSERS.includes(browser.name);
This revision is now accepted and ready to land.Apr 3 2023, 1:57 PM
kamil updated this revision to Diff 24698.Apr 6 2023, 2:40 AM

change conditions

kamil added inline comments.Apr 6 2023, 2:42 AM
web/database/utils/constants.js
11 ↗(On Diff #24551)

yeah, I had "detected as 10" in mind, sorry for confusion

Harbormaster completed remote builds in B18029: Diff 24698.Apr 6 2023, 2:57 AM
Closed by commit rCOMM852d009eb408: [web-db] support SQLite for staff and non-mobile browsers (authored by kamil). · Explain WhyApr 6 2023, 3:06 AM
This revision was automatically updated to reflect the committed changes.
kamil added a commit: rCOMM852d009eb408: [web-db] support SQLite for staff and non-mobile browsers.

Revision Contents
Changeset List

PathSize
web/
database/
utils/
15 lines
23 lines

Diff 24698

View Options

web/database/utils/constants.js

Loading...
View Options

web/database/utils/db-utils.js

Loading...