Skip to content

DRAFT: DON'T MERGE: code review 2#90

Open
webpapaya wants to merge 1 commit intomainfrom
review-branch-ii
Open

DRAFT: DON'T MERGE: code review 2#90
webpapaya wants to merge 1 commit intomainfrom
review-branch-ii

Conversation

@webpapaya
Copy link
Copy Markdown
Collaborator

No description provided.

@webpapaya webpapaya changed the title DON'T MERGE: code review 2 DRAFT: DON'T MERGE: code review 2 Jun 11, 2023
@webpapaya
Copy link
Copy Markdown
Collaborator Author

webpapaya commented Jun 14, 2023

Functionality: 4,5/5

The software should meet the functional requirements specified in the project description. It should work as intended, and all features should be fully implemented and functional.

Creating/editing and viewing recipes works very well. Sharing a recipe works really well. Also from the UX side lots of things improved. I have a feeling you could optimize the image loading as they're pretty slow on my machine.

Code Quality: 4,5/5

The code should be well-structured, and be easily maintainable. It should avoid unnecessary complexity, duplication, and unused code.

The code is structured well and it is easy to understand. Your components folder could be structured better as you have a mix of generic components (eg. TextInput/Button) and application specific components in the same folder.

Security: 3/5

The software should be secure, with appropriate measures in place to protect against common security threats such as SQL injection, cross-site scripting, and cross-site request forgery.

You added security rules to your firestore but the feed and shared collection is still writable by everybody who has some authentication set up. So an attacker could setup an account and then overwrite other peoples recipes.

Presentation: 5/5

Students should be able to present their software project in a clear and concise manner and be able to communicate their ideas and design choices effectively. (eg. during first user story presentation/dailies)

You raised issues during the dailies accordingly and showed your progress during each of them.

Agile Methodology: 5/5

The development should use agile principles, such as iterative development, continuous integration, and testing, frequent customer feedback (eg. ask other teams for their opinion). The team should also demonstrate the ability to work collaboratively.

You showed a really good progress during the 2nd studio week and I saw that you delivered features at a constant pace.

Adapt to changes: 5/5

The team should be able to adapt to changes in the project scope or requirements (eg. feedback from user testing).

  • Create protected route ✅
  • Persist recipe values directly in formik src/components/CreateRecipe.jsx
  • Create dedicated components Input components containing the errors ✅
  • Extract css custom properties or sass variables ✅
  • fix issues mentioned in functionality ✅

User Experience: (to be rated by Melanie)

The user experience should be user-friendly and easy to navigate with a clear, predefined user goal. Suitable usability heuristics should be taken into consideration.

User Interface: (to be rated by Melanie)

The interface should follow design principles that align with the functionality and purpose of the software. The visual language of the interface should be oriented towards state of the art approaches. Interactions, animations and transitions should support the perception of the user goal. (functionality and storytelling)

Testing: 5/5

The software should be tested thoroughly, including unit testing, integration testing, and system testing. The testing should cover all possible use cases, and the results should be documented.

Code Coverage:

All files                        |   60.92 |    57.34 |   61.38 |   61.25 |

You put some effort into your test but some of them are failing and your coverage is below 50%.

.required('Preparation step must not be empty.')
});

const validationSchema = object().shape({
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I would have used a better name here. (eg. recipeSchema)

<div className='delete-popup'>
<div className='delete-popup-inner-container'>
<h1>{title}</h1>
<hr></hr>
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nitpick: <hr/>


test('CreateRecipe renders without crashing', async () => {
await act(async () => {
render(<CreateRecipe />)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Each test should contain an assertion (simply rendering a component is usually not considered a good test). Remember the Arrange/Act/Assert structure from the lecture

@zhva
Copy link
Copy Markdown
Owner

zhva commented Jun 14, 2023

Screenshot 2023-06-14 214705
Screenshot 2023-06-14 214735

Wie hast du das ganze statistic geprüft, weil bei mir sieht das ganz anders aus. Und bei mir gibt es keine durchgefallene Tests.. Ich versuchte alle zu fixen, wenn wir etwas verändert haben..

@zhva
Copy link
Copy Markdown
Owner

zhva commented Jun 14, 2023

You added security rules to your firestore but the feed and shared collection is still writable by everybody who has some authentication set up. So an attacker could setup an account and then overwrite other peoples recipes.

Die recipes in Feed und Shared ja, aber die Rezepte von anderen Users nein.

@zhva
Copy link
Copy Markdown
Owner

zhva commented Jun 14, 2023

I have a feeling you could optimize the image loading as they're pretty slow on my machine.

Ich habe lazy load implementiert sollte schon schneller gehen..

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