Skip to content

Conversation

carlobeltrame
Copy link
Member

@carlobeltrame carlobeltrame commented Aug 21, 2025

Fixes #7481
Fixes #7324
Fixes #7445
Fixes #6281

image image

To be discussed / decided / done:

  • Is the location of the switch in the UI fine, or should it be in the danger zone or on the team page?
  • Write extensive API permission tests for accessing shared camps and all other entities
  • Find all places in the UI where we need to hide / disable things for sharing visitors
    • Activity filtering UI
    • Any "add xyz" buttons
    • Anywhere there is a console error
    • Improve readability of readonly fields
    • more...

@carlobeltrame carlobeltrame force-pushed the shared-camps branch 2 times, most recently from 19845e2 to 5f0de3a Compare August 29, 2025 15:02
@carlobeltrame carlobeltrame added the deploy! Creates a feature branch deployment for this PR label Sep 2, 2025
Copy link

github-actions bot commented Sep 2, 2025

Feature branch deployment ready!

Name Link
😎 Deployment https://pr8004.ecamp3.ch/
🔑 Login [email protected] / test
🕒 Last deployed at Thu Sep 18 2025 14:42:21 GMT+0200
🔨 Latest commit d2137062d49bc21b827c66b3ee4f3146b83ea623
🔍 Latest deploy log https://github.com/ecamp/ecamp3/actions/runs/17828960286/job/50688901760
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

@carlobeltrame carlobeltrame removed the deploy! Creates a feature branch deployment for this PR label Sep 4, 2025
"self": {
"href": "escaped_value"
},
"sharedBy": "escaped_value"
Copy link
Member Author

Choose a reason for hiding this comment

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

Anyone knows why this isn't wrapped in a { "href": ... }?

private static function getContentNodeEndpointQueryCountRanges(): array {
return [
'/content_nodes' => [10, 12],
'/content_nodes' => [11, 13],
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why this went up...

Copy link
Member Author

Choose a reason for hiding this comment

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

/activity_responsibles: 6
/activity_responsibles/item: 8
/camps: 15
/camps: 18
Copy link
Member Author

Choose a reason for hiding this comment

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

What are the numbers in this snapshot? Query count? Why did this go up?

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlobeltrame carlobeltrame force-pushed the shared-camps branch 2 times, most recently from e2f1de3 to 48d92bb Compare September 4, 2025 05:49
{{ $tc(`components.campAdmin.campSharingSettings.copyCampLink`) }}
</v-tooltip>
</v-list-item-action>
<v-list-item-action v-if="isManager" class="align-self-start">
Copy link
Member Author

Choose a reason for hiding this comment

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

As with all camp settings, the frontend only allows managers to change this, but in the API it is also possible for members. #3440

@carlobeltrame carlobeltrame marked this pull request as ready for review September 4, 2025 06:05
@carlobeltrame carlobeltrame changed the title WIP share camps Shared camps Sep 4, 2025
@carlobeltrame carlobeltrame mentioned this pull request Sep 4, 2025
8 tasks
@carlobeltrame carlobeltrame added the deploy! Creates a feature branch deployment for this PR label Sep 5, 2025
@carlobeltrame carlobeltrame force-pushed the shared-camps branch 2 times, most recently from ee508e6 to 3972403 Compare September 6, 2025 01:25
@carlobeltrame carlobeltrame added deploy! Creates a feature branch deployment for this PR and removed deploy! Creates a feature branch deployment for this PR labels Sep 6, 2025
@BacLuc
Copy link
Contributor

BacLuc commented Sep 6, 2025

https://github.com/coderabbitai review this pr

@BacLuc
Copy link
Contributor

BacLuc commented Sep 6, 2025

@coderabbitai review this pr

Copy link

coderabbitai bot commented Sep 6, 2025

@BacLuc I'll review the PR for you, focusing on the shared camps functionality implementation including the API permission changes, UI modifications for sharing visitors, and overall code quality.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

'detail' => 'Access Denied.',
]);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Add tests for setting and un-setting the flag, and check that the sharedSince and sharedBy fields are set correctly.

Copy link
Contributor

@BacLuc BacLuc left a comment

Choose a reason for hiding this comment

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

already very cool, thank you


/**
* Whether the programme of this camp is publicly available to anyone (except for
* personal data such as camp collaborations, personal material lists,
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment might not yet be correct

/**
* Date and time when the camp was last set to be shared publicly.
*/
#[ApiProperty(example: '2025-10-01T00:00:00+00:00', required: true, openapiContext: ['format' => 'date-time'])]
Copy link
Contributor

Choose a reason for hiding this comment

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

why required?

}

public function onBeforeSharingStatusChange(Camp $data): Camp {
if ($data->isShared) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is to keep the timestamp when the camp was last shared and by whom, even if its not shared anymore.

]);
}

public function testCreateActivityInSharedCampIsDeniedForUnrelatedUser() {
Copy link
Contributor

Choose a reason for hiding this comment

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

testCreateActivityInSharedCampIsAllowedForCollaborator?

]);
}

public function testDeleteActivityFromCampSharedIsDeniedForInvitedUser() {
Copy link
Contributor

Choose a reason for hiding this comment

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

testDeleteActivityFromCampSharedIsAllowedForCollaborator

"self": {
"href": "escaped_value"
},
"sharedBy": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too... is this correct

<template>
<div class="e-sortable-checklist-item" @dragstart="gugus">
<ChecklistItemEdit :checklist="checklist" :checklist-item="item">
<div class="e-sortable-checklist-item" @dragstart="startDragging">
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe put into a separate component later

"navigation": {
"desktop": {
"navTopbar": {
"admin": "Admin",
Copy link
Contributor

Choose a reason for hiding this comment

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

were these left over translations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. They were referenced nowhere.

@BacLuc BacLuc requested a review from a team September 6, 2025 14:46
@BacLuc
Copy link
Contributor

BacLuc commented Sep 6, 2025

@coderabbitai review this pr again

Copy link

coderabbitai bot commented Sep 6, 2025

@BacLuc I'll conduct a comprehensive re-review of the entire PR, examining all the changes in the shared camps functionality implementation including the API permissions, UI modifications for sharing visitors, and code quality across all modified files.

✅ Actions performed

Full review triggered.

@carlobeltrame carlobeltrame added deploy! Creates a feature branch deployment for this PR and removed deploy! Creates a feature branch deployment for this PR labels Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy! Creates a feature branch deployment for this PR
Projects
None yet
3 participants