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

[fix]: Redirect to NOT_FOUND page when landing on unactive Object model #10461

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

Conversation

Paribesh01
Copy link

fix: #10064

Copy link
Contributor

github-actions bot commented Feb 24, 2025

Welcome!

Hello there, congrats on your first PR! We're excited to have you contributing to this project.
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.

Generated by 🚫 dangerJS against ea5af8e

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Added a check in RecordIndexPage to prevent access to disabled object views, redirecting users to NotFound page when attempting to access inactive objects.

  • Reordered conditions in /packages/twenty-front/src/pages/object-record/RecordIndexPage.tsx to check objectMetadataItem?.isActive after ensuring objectMetadataItem is defined to prevent potential undefined behavior
  • Added navigation to NotFound page when isActive is false, addressing issue I shouldn't be redirected to Companies when I've disabled Companies object #10064 where users could access disabled company views
  • Consider adding a loading state between the undefined check and active check to prevent flash of content

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 23 to 25
if (!objectMetadataItem?.isActive) {
navigateApp(AppPath.NotFound);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Missing return statement after navigation - component will continue executing

Suggested change
if (!objectMetadataItem?.isActive) {
navigateApp(AppPath.NotFound);
}
if (!objectMetadataItem?.isActive) {
navigateApp(AppPath.NotFound);
return null;
}

@prastoin prastoin changed the title fix: Active [fix]: Redirect to NOT_FOUND page when landing on unactive Object model Feb 26, 2025
@prastoin prastoin self-requested a review February 26, 2025 16:37
Copy link
Contributor

@prastoin prastoin left a comment

Choose a reason for hiding this comment

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

Hello @Paribesh01, thanks for your contribution. You got the main idea !
Left a request regarding some implementation detail

Note: That would be overkill but when entering settings from the company record table page and deactivating the companies, by leaving settings it will redirect to companies page which will redirect to 404. Not a huge plus value here as object model activation and deactivation is IMO not recurrent nor critical

@@ -17,6 +20,11 @@ export const RecordIndexPage = () => {
'main-context-store',
);

if (!objectMetadataItem?.isActive) {
Copy link
Contributor

@prastoin prastoin Feb 26, 2025

Choose a reason for hiding this comment

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

Remark: Combining optional chaining and bang operator is IMO not very readable.
Here this block will entered if objectMetadataItem is undefined and if isActive is falsy

Also this bypass below if block that will now never occurs anymore ( changing component behavior )

  if (!objectMetadataItem.isActive) {
    navigateApp(AppPath.NotFound);
    return null;
  }
  
  if (
    // First assertion is `never`
    isUndefined(objectMetadataItem) ||
    // Nitpick: might wanna use isUndefined and isEmptyString instead 🤔 
    // Should determine if we want a redirection here
    !isNonEmptyString(contextStoreCurrentViewId)
  ) {
    return null;
  }

@Paribesh01
Copy link
Author

Paribesh01 commented Feb 26, 2025

@prastoin rather then redirecting to 404 page we can so message like "This company has been deactivated" in the page.

@prastoin
Copy link
Contributor

prastoin commented Feb 26, 2025

@prastoin rather then redirecting to 404 page we can so message like "This company has been deactivated."

Yep asked product about the expect behavior here in the related issue #10064 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I shouldn't be redirected to Companies when I've disabled Companies object
2 participants