feat: add upload options to reverse shares' shares #185
Conversation
* Add clickable link to reverse share's shares * Ran format
* fix: set link default value to random * fix: add auto EOL and add conventional-changelog package * Apply suggestions from code review --------- Co-authored-by: Elias Schneider <[email protected]>
|
Just fixed the migrations, and in fact, it doesn't add an entry for all the old reverse shares, but like it don't succeed including an option object in the share information, the frontend automatically loads up the "default" options, which displays everything so it's just like before for older shares 😉 |
backend/prisma/migrations/20230701124358_reverse_share_options/migration.sql
Outdated
Show resolved
Hide resolved
|
Awesome! I'll look into it next week :) |
|
@stonith404 |
frontend/src/components/share/modals/showCreateReverseShareModal.tsx
Outdated
Show resolved
Hide resolved
| @@ -50,6 +53,12 @@ const Body = ({ | |||
| sendEmailNotification: false, | |||
| expiration_num: 1, | |||
| expiration_unit: "-days", | |||
|
|
|||
| easyMode: false, | |||
There was a problem hiding this comment.
See my comment about easyMode. If we remove easyMode the file would need to have a refactor.
There was a problem hiding this comment.
Yes you're right, but is it necessary ? Your choice
| }) | ||
| ).data; | ||
| }; | ||
|
|
||
| const getMyReverseShares = async (): Promise<MyReverseShare[]> => { | ||
| return (await api.get("reverseShares")).data; | ||
| const shares = (await api.get("reverseShares")).data; |
There was a problem hiding this comment.
sharesOptions can only be null if the reverse share was created before this update, or I'm wrong? If yes, I would make the migration directly in the databases instead of the frontend.
There was a problem hiding this comment.
I know, but the migration is not able to create the required UUID, maybe we should rather create it in the backend ?
There was a problem hiding this comment.
You're right it's a pain to create migrations in SQL for a SQLite db. But we should try to stick to this method as it handles the whole migration process really well. If we want to do it in the backend we would have to write the logic by our own.
I found an awful way to generate an UUID but it should work:
SELECT
lower(
hex(randomblob(4)) || '-' || hex(randomblob(2)) || '-' || '4' ||
substr(hex( randomblob(2)), 2) || '-' ||
substr('AB89', 1 + (abs(random()) % 4) , 1) ||
substr(hex(randomblob(2)), 2) || '-' ||
hex(randomblob(6))
) id
There was a problem hiding this comment.
I think we would need to add this into the migration, I'll test it right now :
INSERT INTO "ReverseShareOptions" ("id", "shareId", "easyMode", "customLinkEnabled", "passwordEnabled", "descriptionEnabled", "maximalViewsEnabled")
SELECT
lower(
hex(randomblob(4)) || '-' || hex(randomblob(2)) || '-' || '4' ||
substr(hex( randomblob(2)), 2) || '-' ||
substr('AB89', 1 + (abs(random()) % 4) , 1) ||
substr(hex(randomblob(2)), 2) || '-' ||
hex(randomblob(6))
),
id, 0, 1, 1, 1, 1 FROM "ReverseShare";There was a problem hiding this comment.
@stonith404, in fact I have been a bit busy, so I won't be able to finish this right now. I'll do it later or if you want to go on with this, but I'll finish it anyway.
| <Accordion.Item value="description" sx={{ borderBottom: "none" }}> | ||
| <Accordion.Item | ||
| value="description" | ||
| sx={{ |
There was a problem hiding this comment.
I think using the React if statement would be a cleaner solution instead of using css to optionally display items. It would look like this:
{!options.isReverseShare || (options.shareOptions.descriptionEnabled && (
<Accordion.Item
value="description"
sx={{
borderBottom: "none",
}}
>
<Accordion.Control>Description</Accordion.Control>
<Accordion.Panel>
<Stack align="stretch">
<Textarea
variant="filled"
placeholder="Note for the recepients"
{...form.getInputProps("description")}
/>
</Stack>
</Accordion.Panel>
</Accordion.Item>
))}Change my mind if you don't agree :)
There was a problem hiding this comment.
Yes, we can change this indeed.
Co-authored-by: Elias Schneider <[email protected]>
…al.tsx Co-authored-by: Elias Schneider <[email protected]>
No problem, I understand that you want to keep the codebase clean to make it easier to understand and change. |
|
Any updates on this or should I close the PR? |
Hello,
I've just finished the code for the enhancement of the reverse shares. This PR adds the possibility to configure if the configuration fields of the share are shown, yes or no. This PR closes #155
I know that @stonith404 is in exams so he won't be able to review this PR now (and I don't want him to for now) so this will stay open for a while.
I'm not sure all the code is very clear so feel free to make tons of change requests.
Just to explain a bit my work, I've added the migrations to apply the new schema with the new table
ReverseShareOptionsn that is connected 1:1 with aReverseShareentry. This table stores the Boolean values of the options chosen by the reverse share creator when he created it in the frontend, with checkboxes in an accordion in the modal.Then, when a user try to upload a file, the file that checks for the reverse share sends to the index the information about this share, with the options contained, and then, when the user clicks to make the modal, the modal receives the information about the share and then shows the requested fields or not.
I you have any questions or refactor suggestions, feel free to ask.