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
F3376187: D7188.diff
Tue, Nov 26, 10:32 PM
Unknown Object (File)
Fri, Nov 8, 7:41 AM
Unknown Object (File)
Fri, Nov 8, 7:41 AM
Unknown Object (File)
Fri, Nov 8, 7:41 AM
Unknown Object (File)
Thu, Nov 7, 10:48 AM
Unknown Object (File)
Tue, Nov 5, 4:40 AM
Unknown Object (File)
Tue, Nov 5, 2:10 AM
Unknown Object (File)
Tue, Nov 5, 2:10 AM
View All 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
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

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

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

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

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

web/database/utils/db-utils.js
37–38

I would replace this with:

if (!isDev && (!currentLoggedInUserID || !isStaff(currentLoggedInUserID))) {
  return false;
}
40–43

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

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 24551

View Options

web/database/utils/constants.js

Loading...
View Options

web/database/utils/db-utils.js

Loading...