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.

Sign up for GitHub

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

Jump to bottom

Settings and edit bio #356

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

Settings and edit bio #356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26679b2
Fixed overlapping elements, access to edit-bio and settings
Steindt May 15, 2024
6416f36
Removed unusued import
Steindt May 15, 2024
d3a362a
made button color white
alfredgrip May 28, 2024
13397ea
refactored FormData handling for settings
alfredgrip May 28, 2024
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
5 changes: 2 additions & 3 deletions src/routes/(app)/members/[studentId]/edit-bio/+page.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import apiNames from "$lib/utils/apiNames";
import { memberSchema } from "$lib/zod/schemas";
import { error, fail } from "@sveltejs/kit";
import { message, superValidate } from "sveltekit-superforms/server";
import type { Actions, PageServerLoad } from "./$types";
import { authorize } from "$lib/utils/authorization";
import { redirect } from "$lib/utils/redirect";

export const load: PageServerLoad = async ({ locals, params }) => {
const { prisma, user } = locals;
authorize(apiNames.MEMBER.UPDATE, user);
if (!user || !user.memberId) throw redirect(302, "/home");

Comment on lines -10 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? I think it's weird to redirect someone to home screen instead of just showing error. It can make the page seem confusing

Copy link
Collaborator

Choose a reason for hiding this comment

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

You've also removed the verification that a user can actually edit this person's bio.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I see, the old system seems to not check if params.studentId === user.studentId, in which case you should be able to edit. Otherwise you cannot edit your own bio.

const member = await prisma.member.findUnique({
where: {
Expand Down
126 changes: 65 additions & 61 deletions src/routes/(app)/settings/+page.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,30 @@
import type { Actions, PageServerLoad } from "./$types";
import { fail } from "@sveltejs/kit";
import { authorize } from "$lib/utils/authorization";
import apiNames from "$lib/utils/apiNames";
import { redirect } from "$lib/utils/redirect";

export const load: PageServerLoad = async ({ locals }) => {
const { user, prisma } = locals;
authorize(apiNames.MEMBER.UPDATE, user);

const subscriptionSettings = user
? await prisma.subscriptionSetting.findMany({
where: {
memberId: user.memberId,
},
})
: [];
if (user?.memberId == null) throw redirect(302, "/home");

const subscriptionSettings = await prisma.subscriptionSetting.findMany({
where: {
memberId: user.memberId,
},
});
Comment on lines -8 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a user is not logged in, instead of redirecting them we should show an error to be clearer. This redirect doesn't make much sense. throw error(401, "Not logged in") instead (can't remember exact syntax, check other page.server.ts files or even the authorize function

const subscriptions = subscriptionSettings.map((sub) => sub.type);
const pushSubscriptions = subscriptionSettings
.map((sub) => {
if (sub.pushNotification) return sub.type;
})
.filter((t) => t);
const subscribedTags = user
? await prisma.member.findFirst({
where: {
id: user.memberId,
},
select: {
subscribedTags: {},
},
})
: [];
.filter((type): type is string => type != null && type.length > 0);
const subscribedTags = await prisma.member.findFirst({
where: {
id: user.memberId,
},
select: {
subscribedTags: {},
},
});
return {
tags: await prisma.tag.findMany(),
subscribedTags,
Expand All @@ -44,46 +38,56 @@ export const actions: Actions = {
const { user, prisma } = locals;
const form = await request.formData();
if (!user) return fail(401, { form });
Comment on lines 38 to 40
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should probably use zod and superforms to handle type validation, like we do in other form inputs

const subscription: FormDataEntryValue[] = form.getAll("subscription");
const push: FormDataEntryValue[] = form.getAll("push");
const tags: FormDataEntryValue[] = form.getAll("tag");

// Try-catch if for some reason form data isn't correct
try {
await prisma.subscriptionSetting.deleteMany({
where: {
memberId: user.memberId,
},
});
await prisma.subscriptionSetting.createMany({
data: subscription.map((sub) => {
return {
memberId: user.memberId as string,
type: sub.toString(),
pushNotification: push.find(
(tag) => sub.toString() == tag.toString(),
)
? true
: false,
};
}),
});
await prisma.member.update({
where: {
id: user.memberId as string,
},
data: {
subscribedTags: {
set: tags.map((tag) => {
return {
id: tag.toString(),
};
}),
},
},
});
} catch {
return fail(400, { form });
// Some error handling for the form data
// The type received is FormDataEntryValue, which is string | File
// Only string should be possible, but we make TS happy this way
// And it doesn't hurt to check that the values are strings
const push: string[] = [];
for (const v of form.getAll("push")) {
if (typeof v !== "string") return fail(400, { form });
push.push(v);
}
const subscription: string[] = [];
for (const v of form.getAll("subscription")) {
if (typeof v !== "string") return fail(400, { form });
subscription.push(v);
}
const tags: string[] = [];
for (const v of form.getAll("tag")) {
if (typeof v !== "string") return fail(400, { form });
tags.push(v);
Comment on lines +46 to +59
Copy link
Collaborator

Choose a reason for hiding this comment

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

could be replaced with three one-liners like this:
const push: string[] = form.getAll("push").filter((v) => typeof v === "string");

If we want the form to fail for invalid types, I think zod should help with that. Then we don't have to do the hack above and could just do const pus = form.data.push;

}

const memberId = user.memberId;
if (memberId == null) return fail(400, { form });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (memberId == null) return fail(400, { form });
if (memberId == null) return fail(401, { form });

Copy link
Collaborator

Choose a reason for hiding this comment

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

400 is Bad Request, ie something wrong with the form data. 401 is Unauthorized, ie not logged in.

await prisma.subscriptionSetting.deleteMany({
where: {
memberId: memberId,
},
});
await prisma.subscriptionSetting.createMany({
data: subscription.map((sub) => {
return {
memberId: memberId,
type: sub,
pushNotification: push.find((tag) => sub == tag) ? true : false,
};
}),
});
await prisma.member.update({
where: {
id: memberId,
},
data: {
subscribedTags: {
set: tags.map((tag) => {
return {
id: tag,
};
}),
},
},
});
Comment on lines +64 to +91
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably be done in a transaction in-case one of these fails. Because in that case a user might lose all their subscription settings but none would be added or something. I know this isn't written in this PR but always good to improve our code.

Also, we don't have to remove all of the user's settings, only remove the ones not in the form data.

},
};
4 changes: 2 additions & 2 deletions src/routes/(app)/settings/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
<div class="m-2 flex w-full max-w-2xl flex-col items-center lg:p-0">
<h2 class="mb-2 text-2xl font-bold">{m.setting_notification()}</h2>
{#each Object.entries(NotificationSettingType) as notificationSettingType}
<div class="m-2 w-full">
<div class="m-2 w-full sm:pl-8 sm:pr-8 md:pl-4 md:pr-4">
<!-- Web notification -->
<label class="m-2 flex cursor-pointer flex-row justify-between">
<span class="ms-3 text-sm font-medium">
Expand Down Expand Up @@ -96,7 +96,7 @@
<SubscriptionTags {tags} subscribedTags={subscribedTags.subscribedTags} />
</div>
<button
class="btn absolute bottom-0 mb-4 mt-4 w-full max-w-xl bg-primary"
class="btn absolute bottom-0 mb-4 mt-4 w-full max-w-xl bg-primary text-white"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Across the page we have white text color for buttons with primary background. I do, however, think we should btn-primary instead of bg-primary to have consequential styling across the page.

type="submit">{m.setting_apply()}</button
>
</form>
Expand Down
7 changes: 6 additions & 1 deletion src/routes/(app)/settings/SubscriptionTags.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<script lang="ts">
import { languageTag } from "$paraglide/runtime";
import { type Tag } from "@prisma/client";

export let subscribedTags: Tag[];
Expand All @@ -12,7 +13,11 @@
.filter((t) => t != "");
</script>

<div class="flex flex-col items-center sm:grid sm:grid-cols-2">
<div
class={languageTag() == "sv"
? "flex flex-col items-center md:grid md:grid-cols-2"
: "flex flex-col items-center xl:grid xl:grid-cols-2"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the best way to do this? Feels kinda sketchy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's pretty sketchy, the english names are usually longer than the swedish ones but it either looks pretty empty or cramped if you only have 1 of them. Could otherwise decide on a longer settings page by not having notifications beside tags, but there will be a bit more scrolling if that's ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. It looks fine when I look at it, so it's not outright wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should find some solution with auto columns, min-content or something like that. We never know what the tags will have as names in the future.

>
{#each tags as tag}
<div class="m-1 flex">
<label
Expand Down
Loading