Assign a single meta at puzzle creation time #736
Conversation
|
|
The dropdown for meta options are also subject to #672 |
hunts/src/AddPuzzleModal.js
Outdated
| const [name, setName] = React.useState(""); | ||
| const [url, setUrl] = React.useState(""); | ||
| const [assignedMeta, setAssignedMeta] = React.useState("none"); |
There was a problem hiding this comment.
What's wrong with using a falsy value for this?
There was a problem hiding this comment.
Nothing. I'll change this value and the default option value to ""
| @@ -16,8 +16,8 @@ | |||
| "cookies" | |||
| ], | |||
| "host_permissions": [ | |||
| "0.0.0.0:*", | |||
| "http://localhost:*/*", | |||
| "http://*:*/*", | |||
There was a problem hiding this comment.
This feels kind of sketchy, was this just to include 127.0.0.1 or was there more? I guess it's ok as a temporary hack if we need to
There was a problem hiding this comment.
Also, would this potentially get flagged during the Chrome Extension store review?
There was a problem hiding this comment.
@rgossiaux, didn't you say you use 0.0.0.0?
@akirabaruah I actually already included this in the most recent extension upload and it was accepted. However, maybe this is why it took longer.
https://developer.chrome.com/docs/extensions/develop/concepts/match-patterns has Chrome's suggestions. We can include just http://localhost/*, http://127.0.0.1/*, and http://0.0.0.0/*. Their documentation also suggests:
You can also use http://*:*/* to match localhost, IP addresses, and any port.
There was a problem hiding this comment.
Ah thanks for the clarifications. SGTM.
I guess we want the *s because we want to run this extension on any website, i.e. arbitrary puzzle pages?
There was a problem hiding this comment.
No. I'm not authorizing using OAuth. I'm instead reading the csrftoken from the website's cookie. This is the list of URLs whose cookies I can read from. The asterisks are needed for folks various ports if you want to test it locally.
The access to read any website's title and URL is from the permissions's activeTab
There was a problem hiding this comment.
@rgossiaux, didn't you say you use 0.0.0.0?
Yeah I do, I was wondering why you changed to the *:*/* wildcard pattern since it looked like the only missing local development host was 127.0.0.1. I think *:*/* could match any random site, right? I don't think we want to ask people to give us permissions to read their cookies on the whole web if we can avoid it.
There was a problem hiding this comment.
@rgossiaux I thought *:*/* would match anything with a colon but after testing apparently not. I changed it to "http://127.0.0.1/*" in #737
| <Form.Label>Assigned Meta</Form.Label> | ||
| <Form.Control | ||
| as="select" | ||
| value={assignedMeta} |
There was a problem hiding this comment.
If you use a falsy value like null (@rgossiaux's comment above), you can probably change this to value={assignedMeta ?? "none"}.
There was a problem hiding this comment.
I'll just change both to "".
| @@ -16,8 +16,8 @@ | |||
| "cookies" | |||
| ], | |||
| "host_permissions": [ | |||
| "0.0.0.0:*", | |||
| "http://localhost:*/*", | |||
| "http://*:*/*", | |||
There was a problem hiding this comment.
Also, would this potentially get flagged during the Chrome Extension store review?
| meta = get_object_or_404( | ||
| Puzzle, name=request.data["assigned_meta"], hunt=puzzle.hunt | ||
| ) | ||
| puzzle.metas.add(meta) |
There was a problem hiding this comment.
Should we add a unit test for this? (Can be in a separate PR if we want to expedite this one)
There was a problem hiding this comment.
Sure. I'll add a unit test to this PR
| @@ -16,8 +16,8 @@ | |||
| "cookies" | |||
| ], | |||
| "host_permissions": [ | |||
| "0.0.0.0:*", | |||
| "http://localhost:*/*", | |||
| "http://*:*/*", | |||
There was a problem hiding this comment.
Ah thanks for the clarifications. SGTM.
I guess we want the *s because we want to run this extension on any website, i.e. arbitrary puzzle pages?
No description provided.