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

mantisbt#6479: Direct project link #57

Closed
wants to merge 3 commits into from
Closed

mantisbt#6479: Direct project link #57

wants to merge 3 commits into from

Conversation

Jguilbaud
Copy link

Adding possibility to change current project id when linking to list bug page by adding GET parameter in the URL

@dregad
Copy link
Member

dregad commented Aug 13, 2012

First some generic, high-level comments:

  • When working with pull requests, you should ideally create a specific branch for a given PR, and only push to that branch the commits that belong in the PR. This avoids "pollution" or unwanted commits such as f1d6a05 and the subsequent revert you did. Once the pull request has been sent, you should also avoid rebasing the branch.

  • Please follow our Coding conventions.

  • In terms of "wording" the commits - to properly reference issues, so that they get picked up and properly handled by the source integration plugin, you should

    1. have a space before the '#', and
    2. use a recognized keywords (e.g. "fix, fixes, fixed, resolved") instead of "mantisbt".

    It is also good practice to word-wrap the commit message at 72 chars to avoid long lines.

Now to on your fix:

  • Was there a particular reason for duplicating the project-setting code in both view_all_inc.php and view_all_bug_page.php ? It seems redundant since view_all_inc.php is only referenced from view_all_bug_page.php
  • You are systematically calling helper_set_current_project() even if the project id has not changed
  • The code works as it is, but the Project Selection list is not updated when an URL changing the project ID is used; this does not affect the functionality, but causes confusion (i.e. I'm reporting a bug against specified project P2, but the list still shows the previous current project P1) - this is due to the fact that $_COOKIE is only set at the next page load so we need to force that.

I pushed a branch for your review, including your original commit (revised per coding guidelines) and a subsequent one incuding my above remarks. Please test and let me know if that's OK or if you think further changes are required.

@Jguilbaud
Copy link
Author

"When working with pull requests, you should ideally create a specific branch for a given PR, and only push to that branch the commits that belong in the PR" i will do it next time, it was my first pull on github...

your changes are ok for me

@dregad
Copy link
Member

dregad commented Aug 13, 2012

Committed. Thanks for the patch !

@dregad dregad closed this Aug 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants