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

Assign a single meta at puzzle creation time #736

Merged
merged 7 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,16 @@ def create(self, request, **kwargs):

puzzle = serializer.save(hunt=hunt, chat_room=chat_room)

if (
"assigned_meta" in request.data
and request.data["assigned_meta"]
and request.data["assigned_meta"] != "none"
):
meta = get_object_or_404(
Puzzle, name=request.data["assigned_meta"], hunt=puzzle.hunt
)
puzzle.metas.add(meta)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a unit test for this? (Can be in a separate PR if we want to expedite this one)

Copy link
Contributor Author

@maximized maximized Jan 8, 2024

Choose a reason for hiding this comment

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

Sure. I'll add a unit test to this PR


if google_api_lib.enabled():
transaction.on_commit(
lambda: google_api_lib.tasks.create_google_sheets.delay(puzzle.id)
Expand Down Expand Up @@ -449,6 +459,7 @@ def create(self, request, **kwargs):
)
tag, _ = PuzzleTag.objects.get_or_create(name=tag_name, hunt=puzzle.hunt)
if tag.is_meta:
# Look up the meta and then add it to puzzle.metas
meta = get_object_or_404(Puzzle, name=tag.name, hunt=puzzle.hunt)
if is_ancestor(puzzle, meta):
return Response(
Expand Down
6 changes: 3 additions & 3 deletions chrome_extension/cardboard/manifest.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"manifest_version": 3,
"name": "Cardboard Chrome Extension",
"version": "1.0",
"version": "1.0.1",
"icons": {
"16": "images/cardboard_bird.png",
"32": "images/cardboard_bird.png",
Expand All @@ -16,8 +16,8 @@
"cookies"
],
"host_permissions": [
"0.0.0.0:*",
"http://localhost:*/*",
"http://*:*/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, would this potentially get flagged during the Chrome Extension store review?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

"http://localhost/*",
"https://cardboard.rocks/*",
"https://www.cardboard.rocks/*"
]
Expand Down
26 changes: 25 additions & 1 deletion hunts/src/AddPuzzleModal.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
import React from "react";
import { useDispatch } from "react-redux";
import { useDispatch, useSelector } from "react-redux";
import Modal from "react-bootstrap/Modal";
import Button from "react-bootstrap/Button";
import Form from "react-bootstrap/Form";
import { addPuzzle } from "./puzzlesSlice";
import { hideModal } from "./modalSlice";
import { selectHuntTags } from "./huntSlice";

function AddPuzzleModal({ huntId }) {
const allTags = useSelector(selectHuntTags);
const [name, setName] = React.useState("");
const [url, setUrl] = React.useState("");
const [assignedMeta, setAssignedMeta] = React.useState("none");
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with using a falsy value for this?

Copy link
Contributor Author

@maximized maximized Jan 8, 2024

Choose a reason for hiding this comment

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

Nothing. I'll change this value and the default option value to ""

const [isMeta, setIsMeta] = React.useState(false);
const [createChannels, setCreateChannels] = React.useState(true);
const dispatch = useDispatch();
Expand All @@ -21,12 +24,14 @@ function AddPuzzleModal({ huntId }) {
url,
is_meta: isMeta,
create_channels: createChannels,
assigned_meta: assignedMeta,
})
).finally(() => {
dispatch(hideModal());
});
return false;
};

return (
<>
<Modal.Header closeButton>
Expand All @@ -53,6 +58,25 @@ function AddPuzzleModal({ huntId }) {
onChange={(e) => setUrl(e.target.value)}
/>
</Form.Group>
<Form.Group controlId="addPuzzle.meta">
<Form.Label>Assigned Meta</Form.Label>
<Form.Control
as="select"
value={assignedMeta}
Copy link
Contributor

@akirabaruah akirabaruah Jan 8, 2024

Choose a reason for hiding this comment

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

If you use a falsy value like null (@rgossiaux's comment above), you can probably change this to value={assignedMeta ?? "none"}.

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'll just change both to "".

onChange={(e) => setAssignedMeta(e.target.value)}
>
<option key="none" value="none">
None
</option>
{allTags
.filter((tag) => tag.is_meta)
.map((tag, i) => (
<option key={tag.name} value={tag.name}>
{tag.name}
</option>
))}
</Form.Control>
</Form.Group>
<Form.Check
type="checkbox"
label="Is meta"
Expand Down
3 changes: 2 additions & 1 deletion hunts/src/puzzlesSlice.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@ import api from "./api";

export const addPuzzle = createAsyncThunk(
"puzzles/addPuzzle",
async ({ huntId, name, url, is_meta, create_channels }) => {
async ({ huntId, name, url, is_meta, create_channels, assigned_meta }) => {
const response = await api.addPuzzle(huntId, {
name,
url,
is_meta,
create_channels,
assigned_meta,
});
return response;
}
Expand Down
Loading