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

Upgrade to latest Keystone and update copy #369

Merged
merged 12 commits into from
Oct 20, 2022
Merged

Upgrade to latest Keystone and update copy #369

merged 12 commits into from
Oct 20, 2022

Conversation

dcousens
Copy link
Member

@dcousens dcousens commented Oct 20, 2022

I don't know if I'm aligned with the idea that we should ship a default example with access: allowAll, but I digress.
I have at least removed the impression that access control might be in effect by removing the following code:

    ui: {
      // For our starter, we check that someone has session data before letting them see the Admin UI.
      isAccessAllowed: (context) => !!context.session?.data,
    },

If we're going to add an isAccessAllowed to that effect, then we should add actual access control to everything.
For now I have added the following copy to each of the mandatory access definitions:

    // WARNING
    //   for this starter project, anyone can create, query, update and delete anything
    //   if you want to prevent random people on the internet from accessing your data,
    //   you can find out more at https://keystonejs.com/docs/guides/auth-and-access-control

I might follow that up soon, but at least for now the user has plenty of warning and opportunity to learn what to change and where.

@dcousens dcousens self-assigned this Oct 20, 2022
@changeset-bot

This comment was marked as resolved.

@@ -8,22 +8,12 @@
"build": "keystone build",
"postinstall": "keystone postinstall"
},
"// @aws-sdk": "temporary dependency until https://github.com/keystonejs/keystone/issues/8023 is resolved",
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunate, but we can remove it when keystonejs/keystone#8023 is resolved

isIndexed: 'unique',
isFilterable: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

The default is isFilterable: true

posts: relationship({ ref: 'Post.author', many: true }),
},
// Here we can configure the Admin UI. We want to show a user's name and posts in the Admin UI
ui: {
Copy link
Member Author

@dcousens dcousens Oct 20, 2022

Choose a reason for hiding this comment

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

I didn't see a reason to keep this


createdAt: timestamp({
// this sets the timestamp to Date.now() when the user is first created
defaultValue: { kind: 'now' }
Copy link
Member Author

@dcousens dcousens Oct 20, 2022

Choose a reason for hiding this comment

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

As an example of a timestamp, this seemed more appropriate compared to publishDate

ui: {
displayMode: 'segmented-control',
},
}),
Copy link
Member Author

@dcousens dcousens Oct 20, 2022

Choose a reason for hiding this comment

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

I think we can keep this example simple

If we introduce a status to newcomers, we should do something useful with it.
On that point, we should update the blog example to do something with the status (like hiding posts that aren't published).

index.test.ts Outdated Show resolved Hide resolved
'The SESSION_SECRET environment variable must be set in production'
);
} else {
sessionSecret = '-- DEV COOKIE SECRET; CHANGE ME --';
Copy link
Member Author

@dcousens dcousens Oct 20, 2022

Choose a reason for hiding this comment

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

We shouldn't do this, someone will be burnt 🔥 by it, somewhere

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 guess the problem is this will force someone to login every time they update the schema

@dcousens
Copy link
Member Author

dcousens commented Oct 20, 2022

I think we need to decide what this example is meant to be.

The balance at the moment is halfway between a template project (like create-react-app) and a project to learn Keystone with.
I think Keystone is too complicated to try and box up a template project and tutorial in one.

@dcousens dcousens assigned flexdinesh and unassigned dcousens Oct 20, 2022
@dcousens dcousens merged commit 0db3a2b into main Oct 20, 2022
@dcousens dcousens deleted the upgrade branch October 20, 2022 23:34
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.

None yet

2 participants