ashatat / e-commerce

Basic e-commerce website to practice our gitflow, teamwork, communication and clean code best practices.
MIT License
3 stars 1 forks source link

[Enhancement] Proposed solution to blocking PRs. #92

Open FarahZaqout opened 5 years ago

FarahZaqout commented 5 years ago

I think we are doing great splitting features into issues, and we should keep doing it.

However, I think it becomes counterproductive when we are doing 1 issue per PR. We often get stuck waiting for approval/change requests on issues before we can work on others that depend on them.

From this point forward, and for the next project as well, I propose the following:

A PR should relate an entire page. So in the frontend, we split the work into the following

This way, one pull request is open for an entire page. Two or more collaborator can push different features into the same branch if they are part of the same page. Or just one collaborator works on it all. However, we should not find ourselves stuck in features that we know how to code.

In the backend, it works a little similar too. Working on an endpoint cannot proceed until the query that endpoint needs to call is finished.

Therefore, I think the following would be suitable: Backend Sprint 1: Database

Sprint 2: controllers

should be up in their own sprint. Either that or having an entire endpoint and doing its models, queries and controllers, which I think is bad.

FarahZaqout commented 5 years ago

Of course, this means that we will still work on one small issue at a time, and once the feature is finished, we can just remove in progress label and move on to another while awaiting review for it.

One problem with this idea is the question of how a reviewer would know which code is for which issue.

I think this comes down to how well we comment our code. as the issue will be related in the PR description, a review will be required every time an issue is finished, and by reading the relate, we should be able to figure out which bits of the code are relevant for review by reading the comments in the code.

As the PR will relate multiple issues, with each new review request, we can add (current) next to the relate.

so for example

222 {current)

221(current)

444

What do you guys think?

FarahZaqout commented 5 years ago

@amusameh @ashatat @RamyAlshurafa @ishak52

ashatat commented 5 years ago

@FarahZaqout It'd be messy if we push more than one feature on the same PR, file changes would be messy, and it'd be hard to review. but I hear you about blocking PRs. I think the solution relies on architecture the app well so you'd know what is your priorities. so the PRs that would be needed should be worked first ((we should ask a senior developer)), I am sure they have a way.

FarahZaqout commented 5 years ago

That's a good idea. I wonder if Tom might have something to say about this. Can @amusameh contact him for us?

FarahZaqout commented 5 years ago

In regards to the routes, I will open an issue now and everyone should write their endpoints in it, then one of us can just work on it and get it up today/tomorrow.