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

[WEB-2500] feat: Product updates modal (What's new in Plane) #5690

Draft
wants to merge 7 commits into
base: preview
Choose a base branch
from
Draft

[WEB-2500] feat: Product updates modal (What's new in Plane) #5690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c455bbe
[WEB-2500] feat: Product updates modal (What's new in Plane)
prateekshourya29 Sep 24, 2024
2b0f835
fix: build errors.
prateekshourya29 Sep 24, 2024
17f3603
Merge branch 'preview' of github.com:makeplane/plane into feat/produc…
prateekshourya29 Sep 24, 2024
84ca48e
fix: merge conflicts resolved
prateekshourya29 Oct 4, 2024
dae6133
fix: lint errors resolved.
prateekshourya29 Oct 4, 2024
9c3f825
chore: minor improvements.
prateekshourya29 Oct 4, 2024
171a95f
Merge branch 'preview' of github.com:makeplane/plane into feat/produc…
prateekshourya29 Oct 4, 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
1 change: 1 addition & 0 deletions apiserver/Dockerfile.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ FROM python:3.12.5-alpine AS backend
ENV PYTHONDONTWRITEBYTECODE 1
ENV PYTHONUNBUFFERED 1
ENV PIP_DISABLE_PIP_VERSION_CHECK=1
ENV INSTANCE_CHANGELOG_URL https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Hardcoding INSTANCE_CHANGELOG_URL May Expose Sensitive Information

The environment variable INSTANCE_CHANGELOG_URL is currently hardcoded in the Dockerfile with a URL containing an identifier (8e1c2e4c7bc5493eb7731be3862f6960) that appears to be sensitive. To enhance security and flexibility, consider the following changes:

  • Use a Placeholder for the URL: Replace the hardcoded URL with a placeholder in the Dockerfile.
  • Inject the Actual URL at Runtime: Set the real URL through environment variables during container deployment or runtime configuration.

Suggested Change:

- ENV INSTANCE_CHANGELOG_URL https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/
+ ENV INSTANCE_CHANGELOG_URL ${INSTANCE_CHANGELOG_URL:-https://api.plane.so/api/public/anchor/PLACEHOLDER/pages/}

Benefits:

  1. Security: Prevents exposure of sensitive identifiers in the codebase.
  2. Flexibility: Allows different environments (development, staging, production) to use unique URLs without modifying the Dockerfile.
  3. Maintainability: Simplifies updates to the URL without needing to rebuild the Docker image.
Analysis chain

Consider using a placeholder for the INSTANCE_CHANGELOG_URL

While adding this environment variable aligns with the PR objectives for implementing a product updates modal, hardcoding the full URL in the Dockerfile might pose a security risk, especially since it contains what appears to be an API key or identifier (8e1c2e4c7bc5493eb7731be3862f6960).

Consider replacing the hardcoded URL with a placeholder and injecting the actual URL at runtime or during deployment. This approach enhances security and flexibility. Here's a suggested change:

-ENV INSTANCE_CHANGELOG_URL https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/
+ENV INSTANCE_CHANGELOG_URL ${INSTANCE_CHANGELOG_URL:-https://api.plane.so/api/public/anchor/PLACEHOLDER/pages/}

This change allows you to:

  1. Set a default placeholder URL in the Dockerfile.
  2. Override it with the actual URL during container runtime or in your deployment configuration.
  3. Avoid exposing potentially sensitive information in your Dockerfile.

To ensure this environment variable is used correctly in the application code, please run the following command:

This will help verify that the application is properly configured to use this new environment variable.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of INSTANCE_CHANGELOG_URL in Python files
rg --type python 'INSTANCE_CHANGELOG_URL' -C 3

Length of output: 77

Script:

#!/bin/bash
# Search for usage of INSTANCE_CHANGELOG_URL in Python files using the correct file type alias
rg --type py 'INSTANCE_CHANGELOG_URL' -C 3

Length of output: 1986


WORKDIR /code

Expand Down
1 change: 1 addition & 0 deletions apiserver/Dockerfile.dev
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ FROM python:3.12.5-alpine AS backend
ENV PYTHONDONTWRITEBYTECODE 1
ENV PYTHONUNBUFFERED 1
ENV PIP_DISABLE_PIP_VERSION_CHECK=1
ENV INSTANCE_CHANGELOG_URL https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Hardcoded INSTANCE_CHANGELOG_URL detected in Dockerfiles

The INSTANCE_CHANGELOG_URL environment variable is hardcoded in the following Dockerfiles, which may pose security risks and reduce flexibility across different environments:

  • apiserver/Dockerfile.dev
  • apiserver/Dockerfile.api

Recommendations:

  1. Replace the hardcoded URL with environment variable placeholders to allow configuration at runtime: 
    ENV INSTANCE_CHANGELOG_URL ${INSTANCE_CHANGELOG_URL}
  2. Store the actual URL in environment-specific configuration files or secure storage solutions such as Kubernetes Secrets or CI/CD pipeline variables.
  3. Ensure that sensitive tokens or identifiers within URLs are managed securely and are not exposed in the codebase.
Analysis chain

Approve with suggestions: Consider security and flexibility for INSTANCE_CHANGELOG_URL

The addition of the INSTANCE_CHANGELOG_URL environment variable aligns well with the PR objectives for implementing the "What's new in Plane" feature. However, there are some considerations to improve security and flexibility:

  1. Instead of hardcoding the actual URL in the Dockerfile, consider using a placeholder value:

    ENV INSTANCE_CHANGELOG_URL ${INSTANCE_CHANGELOG_URL:-https://api.example.com/changelog}

    This allows for easy overriding of the URL at runtime or through environment-specific configurations.

  2. Move the actual URL setting to a more secure and flexible method, such as:

    • Environment-specific .env files
    • Kubernetes ConfigMaps or Secrets
    • CI/CD pipeline variables

  3. Please verify:

    • Is this URL sensitive? The long alphanumeric string might be a token or identifier.
    • Does this URL change across different environments (dev, staging, production)?

These changes will enhance security and make the configuration more adaptable to different deployment scenarios.

To ensure this URL isn't used elsewhere in the codebase, run:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for hardcoded instances of the changelog URL
rg --type-not dockerfile "https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/"

Length of output: 140

Script:

#!/bin/bash
# Search for hardcoded instances of the changelog URL excluding Dockerfiles
rg --ignore-file Dockerfile "https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/"

Length of output: 421


RUN apk --no-cache add \
"bash~=5.2" \
Expand Down
2 changes: 2 additions & 0 deletions apiserver/plane/license/api/views/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@
InstanceAdminSignOutEndpoint,
InstanceAdminUserSessionEndpoint,
)

from .changelog import ChangeLogEndpoint

Check notice on line 22 in apiserver/plane/license/api/views/__init__.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

apiserver/plane/license/api/views/__init__.py#L22 

'.changelog.ChangeLogEndpoint' imported but unused (F401)
35 changes: 35 additions & 0 deletions apiserver/plane/license/api/views/changelog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Python imports
import requests

# Django imports
from django.conf import settings

# Third party imports
from rest_framework.response import Response
from rest_framework import status
from rest_framework.permissions import AllowAny

# plane imports
from .base import BaseAPIView


class ChangeLogEndpoint(BaseAPIView):
permission_classes = [
AllowAny,
]

def fetch_change_logs(self):
response = requests.get(settings.INSTANCE_CHANGELOG_URL)

Check warning on line 22 in apiserver/plane/license/api/views/changelog.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

apiserver/plane/license/api/views/changelog.py#L22 

Requests call without timeout

Check warning on line 22 in apiserver/plane/license/api/views/changelog.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

apiserver/plane/license/api/views/changelog.py#L22 

The application was found using the `requests` module without configuring a timeout value for connections.
response.raise_for_status()
return response.json()
Comment on lines +21 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and logging to fetch_change_logs method.

While the method correctly raises an exception for HTTP errors, it lacks specific error handling and logging. This could make debugging issues more difficult in production.

Consider adding try-except blocks to handle potential exceptions and log errors. Here's a suggested implementation:

import logging

logger = logging.getLogger(__name__)

def fetch_change_logs(self):
    try:
        response = requests.get(settings.INSTANCE_CHANGELOG_URL)
        response.raise_for_status()
        return response.json()
    except requests.RequestException as e:
        logger.error(f"Error fetching changelog: {str(e)}")
        raise

This change will log any errors that occur during the request, making it easier to diagnose issues in production.


def get(self, request):
# Fetch the changelog
if settings.INSTANCE_CHANGELOG_URL:
data = self.fetch_change_logs()
return Response(data, status=status.HTTP_200_OK)
else:
return Response(
{"error": "could not fetch changelog please try again later"},
status=status.HTTP_400_BAD_REQUEST,
)
Comment on lines +26 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and status codes in get method.

The current implementation has a few areas that could be improved:

  1. The error message when INSTANCE_CHANGELOG_URL is not set could be more specific.
  2. Returning HTTP 400 (Bad Request) when the URL is not set might not be the most appropriate status code.
  3. There's no error handling for potential exceptions from fetch_change_logs.

Consider the following improvements:

from django.core.exceptions import ImproperlyConfigured

def get(self, request):
    if not settings.INSTANCE_CHANGELOG_URL:
        raise ImproperlyConfigured("INSTANCE_CHANGELOG_URL is not set in settings.")
    
    try:
        data = self.fetch_change_logs()
        return Response(data, status=status.HTTP_200_OK)
    except requests.RequestException as e:
        return Response(
            {"error": f"Failed to fetch changelog: {str(e)}"},
            status=status.HTTP_503_SERVICE_UNAVAILABLE
        )

This implementation:

  1. Raises an ImproperlyConfigured exception if the URL is not set, which is more appropriate for configuration issues.
  2. Handles potential exceptions from fetch_change_logs.
  3. Returns a more appropriate HTTP 503 (Service Unavailable) status when the changelog fetch fails.

2 changes: 2 additions & 0 deletions apiserver/plane/license/api/views/instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ def get(self, request):
data["space_base_url"] = settings.SPACE_BASE_URL
data["app_base_url"] = settings.APP_BASE_URL

data["instance_changelog_url"] = settings.INSTANCE_CHANGELOG_URL

instance_data = serializer.data
instance_data["workspaces_exist"] = Workspace.objects.count() >= 1

Expand Down
6 changes: 6 additions & 0 deletions apiserver/plane/license/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
InstanceAdminUserMeEndpoint,
InstanceAdminSignOutEndpoint,
InstanceAdminUserSessionEndpoint,
ChangeLogEndpoint
)

urlpatterns = [
Expand All @@ -19,6 +20,11 @@
InstanceEndpoint.as_view(),
name="instance",
),
path(
"changelog/",
ChangeLogEndpoint.as_view(),
name="instance-changelog",
),
path(
"admins/",
InstanceAdminEndpoint.as_view(),
Expand Down
3 changes: 3 additions & 0 deletions apiserver/plane/settings/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,3 +359,6 @@
APP_BASE_URL = os.environ.get("APP_BASE_URL")

HARD_DELETE_AFTER_DAYS = int(os.environ.get("HARD_DELETE_AFTER_DAYS", 60))

# Instance Changelog URL
INSTANCE_CHANGELOG_URL = os.environ.get("INSTANCE_CHANGELOG_URL", "")
9 changes: 2 additions & 7 deletions web/app/profile/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,8 @@ const ProfileSettingsPage = observer(() => {
ref={ref}
hasError={Boolean(errors.email)}
placeholder="Enter your email"
className={`w-full cursor-not-allowed rounded-md !bg-custom-background-80 ${
errors.email ? "border-red-500" : ""
}`}
className={`w-full cursor-not-allowed rounded-md !bg-custom-background-80 ${errors.email ? "border-red-500" : ""
}`}
autoComplete="on"
disabled
/>
Expand Down Expand Up @@ -436,8 +435,4 @@ const ProfileSettingsPage = observer(() => {
);
});

// ProfileSettingsPage.getLayout = function getLayout(page: ReactElement) {
// return <ProfileSettingsLayout>{page}</ProfileSettingsLayout>;
// };

export default ProfileSettingsPage;
3 changes: 1 addition & 2 deletions web/ce/components/global/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
export * from "./version-number";
export * from "./product-updates";
export * from "./product-updates-modal";
export * from "./product-updates-header";
26 changes: 26 additions & 0 deletions web/ce/components/global/product-updates-header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { observer } from "mobx-react";
import Image from "next/image";
// helpers
import { cn } from "@/helpers/common.helper";
// assets
import PlaneLogo from "@/public/plane-logos/blue-without-text.png";
// package.json
import packageJson from "package.json";
Comment on lines +1 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using an environment variable for the package version.

The direct import of package.json might cause issues in certain build configurations, especially in production environments. Consider using an environment variable to store and access the package version instead.

You could set up an environment variable in your build process and access it like this:

const version = process.env.NEXT_PUBLIC_APP_VERSION || 'unknown';

Then use version instead of packageJson.version in your component.


export const ProductUpdatesHeader = observer(() => (
<div className="flex gap-2 mx-6 my-4 items-center justify-between flex-shrink-0">
<div className="flex w-full items-center">
<div className="flex gap-2 text-xl font-medium">What&apos;s new</div>
<div
className={cn(
"px-2 mx-2 py-0.5 text-center text-xs font-medium rounded-full bg-custom-primary-100/20 text-custom-primary-100"
)}
>
Version: v{packageJson.version}
</div>
</div>
<div className="flex flex-shrink-0 items-center gap-8">
<Image src={PlaneLogo} alt="Plane" width={24} height={24} />
</div>
</div>
));
9 changes: 0 additions & 9 deletions web/ce/components/global/product-updates-modal.tsx
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions web/ce/components/global/product-updates.tsx
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion web/core/components/common/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export * from "./product-updates-modal";
export * from "./empty-state";
export * from "./latest-feature-block";
export * from "./breadcrumb-link";
Expand Down
111 changes: 0 additions & 111 deletions web/core/components/common/product-updates-modal.tsx
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions web/core/components/global/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./product-updates";
62 changes: 62 additions & 0 deletions web/core/components/global/product-updates/footer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import Image from "next/image";
// ui
import { getButtonStyling } from "@plane/ui";
// helpers
import { cn } from "@/helpers/common.helper";
// assets
import PlaneLogo from "@/public/plane-logos/blue-without-text.png";

export const ProductUpdatesFooter = () => (
<div className="flex items-center justify-between flex-shrink-0 gap-4 m-6 mb-4">
<div className="flex items-center gap-2">
<a
href="https://go.plane.so/p-docs"
target="_blank"
className="text-sm text-custom-text-200 hover:text-custom-text-100 hover:underline underline-offset-1 outline-none"
>
Docs
</a>
<svg viewBox="0 0 2 2" className="h-0.5 w-0.5 fill-current">
<circle cx={1} cy={1} r={1} />
</svg>
<a
href="https://go.plane.so/p-changelog"
target="_blank"
className="text-sm text-custom-text-200 hover:text-custom-text-100 hover:underline underline-offset-1 outline-none"
>
Full changelog
</a>
<svg viewBox="0 0 2 2" className="h-0.5 w-0.5 fill-current">
<circle cx={1} cy={1} r={1} />
</svg>
<a
href="mailto:support@plane.so"
target="_blank"
className="text-sm text-custom-text-200 hover:text-custom-text-100 hover:underline underline-offset-1 outline-none"
>
Support
</a>
<svg viewBox="0 0 2 2" className="h-0.5 w-0.5 fill-current">
<circle cx={1} cy={1} r={1} />
</svg>
<a
href="https://go.plane.so/p-discord"
target="_blank"
className="text-sm text-custom-text-200 hover:text-custom-text-100 hover:underline underline-offset-1 outline-none"
>
Discord
</a>
</div>
<a
href="https://plane.so/pages"
target="_blank"
className={cn(
getButtonStyling("accent-primary", "sm"),
"flex gap-1.5 items-center text-center font-medium hover:underline underline-offset-2 outline-none"
)}
>
<Image src={PlaneLogo} alt="Plane" width={12} height={12} />
Powered by Plane Pages
</a>
</div>
);
2 changes: 2 additions & 0 deletions web/core/components/global/product-updates/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from "./modal";
export * from "./footer";
Loading
Loading