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

feat: seed with service #75

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

feat: seed with service #75

wants to merge 2 commits into from

Conversation

yamcodes
Copy link
Contributor

@yamcodes yamcodes commented Oct 21, 2023

Description

This PR suggests treating the seeder as a service controller rather than as a direct database manipulator.

Having the seeding device interact with the service layer achieves 2 goals:

  1. Specifically taking advantage of password hashing that is already done in the service layer, instead of reimplementing it
  2. In general, relying on the established 3-tier architecture is a cleaner and more maintainable approach that aligns with good software architecture practices. This way, any logic, validation, and error handling associated with user creation is applied consistently, whether the users are being created as part of regular application use or for seeding purposes.

PR Checklist (Please do not remove)

  • Read the CONTRIBUTING guide
  • Title this PR according to the type(scope): description or type: description format
  • Provide description sufficient to understand the changes introduced in this PR, and, if necessary, some screenshots
  • Reference an issue or discussion where the feature or changes have been previously discussed
  • Add a failing test that passes with the changes introduced in this PR, or explain why it's not feasible
  • Add documentation for the feature or changes introduced in this PR to the docs; you can run them with bun docs

@yamcodes yamcodes changed the title feat: service layer seed feat: seed with service Oct 21, 2023
Copy link
Collaborator

@Hajbo Hajbo left a comment

Choose a reason for hiding this comment

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

Good idea, I really like the direction we are going!

Comment on lines +20 to +30
await usersService.createUser({
email: faker.internet.email(),
username: faker.internet.userName(),
password: await Bun.password.hash(faker.internet.password()),
bio: faker.lorem.text(),
image: faker.image.imageUrl(),
};
console.log('Upserting user:', data);
password: faker.internet.password(),
});

await db.insert(users).values(data);
console.log('User upserted');
// Update the user's bio and image
await usersService.updateUser(i, {
bio: faker.lorem.paragraph(),
image: faker.internet.avatar(),
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should be able to create a user with all attributes. It's just that the frontend won't send the two optional keys in the registration request

Comment on lines +78 to +83
async findAll() {
const users = await this.repository.findAll();
return await Promise.all(
users.map((user) => this.generateUserResponse(user)),
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see two problems with this:

  • It'll generate a valid token for ALL users, which is probably not a good thing to do regardless if it's exposed in an API or not
  • Although the spec doesn't talk about a functionality like this, but I think the returned data format should be changed. Instead of a list of singular user objects ({user: {...}}[]) it could be one plural users object, e.g. {users: [user1, user2]}

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.

2 participants