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

Mino feat explore thumbnial #21

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Conversation

kakizaki55
Copy link
Contributor

@kakizaki55 kakizaki55 commented Apr 28, 2022

-Added avatar to the find all project model using inner joins
-also limited the project to 25 as to not overpopulate the explore page

so something weird is happening with the test and I could seem to get it working locally, its the "gets all projects in the table test" and looking at it, the update to the findAllProjects() function should not have interfered with the test if someone can take a look at the changes I made and see if there is anything that pops out let me know.
Screen Shot 2022-04-28 at 12 38 11 PM

Copy link
Contributor

@jlaurentpdx jlaurentpdx left a comment

Choose a reason for hiding this comment

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

From our conversation in Slack:

...the initial test [does] not include a line for creating a new user profile. When trying to access the user in the profiles table with an Inner Join, no projects were returned for this user because they did not have information in the Profiles table.

Two solutions that come to mind:

  1. Replace INNER JOIN on line 47 of Project.js with LEFT JOIN.
  2. In projects.test.js, update the test 'gets all projects in the table' as follows:
it('gets all projects in the table', async () => {
    const user = await UserService.create(mockUser);
    await agent.post('/api/v1/users/sessions').send(mockUser);
    await agent.post('/api/v1/profiles').send({
      userId: '2',
      username: 'mockusername',
      bio: 'bio',
      avatar: 'url',
    });
    await agent.post('/api/v1/projects').send(user.userId);
    await agent.post('/api/v1/projects').send(user.userId);

    const res = await request(app).get('/api/v1/projects');
    expect(res.body).toHaveLength(3);
  });

Solution 1 will return all projects by any user with or without a profile. Noting that the user should have a profile before creating their first project but can currently create projects without a profile by a POST call to the correct back-end route, this solution seems less-than-ideal.

Solution 2 again relies on the user to have a profile, but will render their projects on the Explore page only if they have a profile. A question this raises is what should appear if the link to the user's avatar is broken?

Either of these solutions could benefit from conditionally rendering the image based on whether profile/avatar returns null or contains a broken image link.

Copy link
Contributor

@jlaurentpdx jlaurentpdx left a comment

Choose a reason for hiding this comment

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

Really appreciate this - the avatars are a nice touch to distinguish each user's projects, and the joins here open up a lot of potential for retrieving extra user information to display on this page in the future. Nice work!

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