Jump to content

Let's Review Pull Requests!


Sylvain CM

Recommended Posts

Have you submitted code to PrestaShop’s Core codebase or any of its native module through a pull request on GitHub? Then read on!

 

The PrestaShop team has four main ways to get feedback from the Community:

  • The Support Forums, where we try to answer the most pressing matters (and there’s never enough hours in the day). Hey, that’s here!
  • The Forge, our bugtracker, where users (both developers and merchants) report issues or suggest improvements, and the team handles them one by one, according to their complexity and impact.
  • The GitHub repository, where developers can submit code changes to the team through pull requests (hereafter “PR”). The repo contains the code for the Core of PrestaShop, as well as individual projects for each module.
  • The comments on the Build devblog.

As anyone lurking on our GitHub repository knows, PRs have gotten from a trickle to a more steady flow of code suggestions, currently reaching more than 360 PRs just for the main project. Modules also have their share of PRs waiting.

 

While we are more than happy to see the Community involved, the current plans for version 1.7 of PrestaShop made it that until now, the devteam had little time to dive into all these PRs and separate the wheat from the chaff. I write “until now”, because we want to dedicate more time to our Community.

 

That’s where you, dear reader who contributed one or more PRs, can easily help us! If one of your past PR is still open, we want to help you either merge it into Core, or close it. Now’s the time to speak up – and comment :) Please read on!

 

How to proceed

 

Look at the pull requests that you have created in the past and ask yourself:

  • Is this still relevant?
  • Does this still need to be fixed?
  • Is the suggestion still valid?

If you answer “no” to any of the above questions, please leave a comment on GitHub explaining the situation, and close the pull request.

 

If you have answered “yes” to all the above questions, then let’s continue!

 

Now, please look at your code, and ask yourself:

  • Do the code changes you suggested at the time still apply?
  • Is there any conflict risk with the current code?
    GitHub should indicate this at the bottom of the PR’s page.
  • Can the solution be improved?
  • Does your patch follow the PSR-2 coding style?
    We adopted PSR-2 back in June 2015.
  • Is the PR made on the correct branch?
    It should be:
    • develop for an improvement or a bug fix targeted at the forthcoming 1.7.
    • 1.6.1.x for a bug fix for version 1.6.1.x (currently 1.6.1.2). PrestaShop 1.6 will not receive any new feature.
    • dev for a PR made on a module.

 

Essential links

 

Here is a link to all the currently open pull requests in all of PrestaShop’s branches:https://github.com/PrestaShop/PrestaShop/pulls. Remember that each module also has its own set of PRs!

 

Here’s how you can display all your pull requests submitted to the PrestaShop project:

  1. Display all your currently open pull requests: https://github.com/pulls.
  2. Open the “Organization” filter and choose “PrestaShop”.

 

A few tips

 

If a rewrite of your patch is needed, either because you can think of a better solution, or because PrestaShop’s codebase has changed since your proposal and thus your patch cannot be applied as-is, please take the time to upgrade your code! It would be of tremendous help to provide an updated patch, one that we can possibly apply more quickly.

 

If you aim at fixing version 1.5 or below, please port your code to the 1.6.1.x or develop branch. PrestaShop’s versions below 1.6 will not receive updates anymore, except for security issues.

 

By following these simple suggestions, you can help us go through the open PRs quicker, spot the real gems faster, and thus make PrestaShop a better e-commerce solution!

 

In any case, please leave a comment acknowledging that you aware of the PR’s situation, and willing to start working with us to have it possibly merged!

 

Thank you for giving us an update on every one of your open pull requests!

Link to comment
Share on other sites

That’s where you, dear reader who contributed one or more PRs, can easily help us! If one of your past PR is still open, we want to help you either merge it into Core, or close it. Now’s the time to speak up – and comment  :) Please read on!

 

How to proceed

 

Look at the pull requests that you have created in the past and ask yourself:

  • Is this still relevant?
  • Does this still need to be fixed?
  • Is the suggestion still valid?
If you answer “no” to any of the above questions, please leave a comment on GitHub explaining the situation, and close the pull request.

 

 

 

Leaving requests open is how you get people to stop contributing to the project. Look at the forge, how many community leaders contribute to that now? Hardly any these days because bugs were ignored and not fixed. This is not how successful systems are run. 

 

Check this bug report out, http://forge.prestashop.com/browse/PSCSX-4818 Open 8 months, is a 10 minute fix. 

 

Still not answered, http://forge.prestashop.com/browse/NM-498

 

Too lazy to fix, http://forge.prestashop.com/browse/PSCSX-3887  It only takes 5 lines of code.

 

Marked as fixed but never tested, http://forge.prestashop.com/browse/PSCSX-3485

 

Closed because he was too busy to set up a test, http://forge.prestashop.com/browse/PSCSX-1907

 

Edit: I realize you are talking about github, but the same principal applies by making people feel left out by their commits not being included.

  • Like 1
Link to comment
Share on other sites

Yes Dh42, we are aware that with 1.7 taking a more prominent part of our development time, we have been missing a lot of great suggestions.

 

And that is exactly what we want to work through: we sure don't want to prevent the Community from reporting issues or from helping us improve the code, and indeed our recent lack of answer on several tickets and pull requests could prevent Community members from wanting to contribute again. We want to fix that!

 

Hence that post on the Build devblog, and the recent automated message on the Forge (on the one coming on PRs): there are so many things to take into account, and so little hours in the day, that we ask those who submitted tickets or commits to give us an insight on their thinking, in order to see where we should focus our effort in the massive amount of data to analyze.

 

You list 5 of your tickets, and that can seem little to review to you, but there are several hundreds to go through. Some issues might only require a 10-minute fix, but that doesn't count the time to get the mind into the context, put oneself in your shoes, understand your thinking, code the whole thing making sure it doesn't break anything else elsewhere, submit it to our QA staff who'll make sure that indeed nothing is broken (and that process takes a whole other level of time), etc.

 

I'm just giving a sample of the mindset needed to go through each ticket, but you get the idea: in order to get those tickets fixed faster, in order to get those commits merged before they are obsolete, we are putting out a call for an update to all those who've been helping us with their time these last few months.

 

Thank you for your help, and see you in Forge or on GitHub!

 

Best regards,

 

-xb.

  • Like 2
Link to comment
Share on other sites

I very much agree with DH42. The current Presta version has severe (Multistore) bugs in it, but the focus with the dev team is already on 1.7

 

It would be really great to have a version that actually works, and move on from there. Just an example; the multistore sitemap for store A has the url for store B. The fix is already months old, but still not included. Not to mention the bug that resets all store setting for products to the main store, making multistore useless (or very, very time consuming)

 

I understand is really exciting to work on new things, but for the store owners, we really like to see some annoying bugs fixed first.

  • Like 1
Link to comment
Share on other sites

  • 3 weeks later...

I have to agree as well...

 

There seems to be a constant drive to add more features while the current feature set is far from bug-free (quite the opposite unfortunately), to add major versions when the current version is far from perfect. Befitting an open-source project, people do their best to report these issues on the forge and get largely ignored, those who can make pull requests for fixes and... get largely ignored. I understand that it's much sexier to add things than fix them but this is getting quite frustrating.

 

My (and your) customers need a working, stable and secure e-commerce system with preferably as many options and features as possible. But number of features should not take priority over quality and for a long time now I have the feeling that this is the case.

 

When 1.7 gets released (early 2016 at best?) it 'll be at least months, but probably closer to a year before it's really stable and production-ready (based on past experience). By that time, we'll have a system that's as reliable as 1.6 is now and work on 1.8 will probably start... In the meantime, we're stuck with 1.6 where so little is getting fixed while so much is available. If the low-hanging fruit would be hanging even lower, it would basically be lying on the ground. It's these choices and priorities that make it so difficult to keep recommending PrestaShop as the e-commerce solution of choice. (done ranting now...)

Link to comment
Share on other sites

I have purposed this many times and I would like to purpose it again. 

 

With major packages that are successful they do one thing when releasing a new version. Its called a feature freeze. Look at how PHP does it, https://wiki.php.net/todo/php70  When the first beta of PHP 7 was released, there was a feature freeze, meaning every new release was just a bug fixing release. Why can we not have this? 

 

I often see features come and go from one subversion to the next. Like the html purifier, 4 subversions have it controllable by the back office 1 or 2 does not, then it is back. Why?

 

I honestly think someone else needs to take control of the development process and managing the developers. They are just not being managed effectively. 

  • Like 1
Link to comment
Share on other sites

  • 2 weeks later...

Is anyone from the Prestateam reading along??  Looks like at this point, no one is caring about the 1.6 branch anymore! As stated by Dh42, the priorities for the developers are just not right. Fixing important bugs should be prio 1. 

 

Does the team realize people are building a business on their product?

Link to comment
Share on other sites

  • 2 months later...

Is anyone from the Prestateam reading along??  Looks like at this point, no one is caring about the 1.6 branch anymore! As stated by Dh42, the priorities for the developers are just not right. Fixing important bugs should be prio 1. 

 

Does the team realize people are building a business on their product?

 

The lack of response confirms the post...

  • Like 1
Link to comment
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
×
×
  • Create New...