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.

Sign up for GitHub

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

Jump to bottom

Adding Issues by project to MantisGraph #1957

Merged
merged 2 commits into from
Feb 18, 2024
Merged

Adding Issues by project to MantisGraph #1957

merged 2 commits into from
Feb 18, 2024

Conversation

CasN
Copy link
Contributor

@CasN CasN commented Jan 18, 2024 •
edited by dregad

This addresses issue 33521

CasN and others added 2 commits February 19, 2024 00:47
@CasN @dregad
MantisGraph: new "By Project" graphs
145a332 
Fixes #33521

Signed-off-by: Damien Regad <dregad@mantisbt.org>

Original commit modified
- Whitespace fixes
- PHPdoc adjustments
- Renamed, reordered and corrected language strings
@dregad
Fix active tab following change of default graph
c03fa0c 
Before introduction of the By Projects graphs, the Graphs menu was
opening the By Developers graph by default.

Since the default was changed to By Projects, it needs to be reflected
in all graph scripts to ensure the Graphs tab is shown as active.

Fixes #33521
@dregad dregad self-assigned this Feb 18, 2024
dregad
dregad approved these changes Feb 18, 2024
View reviewed changes
Copy link
Member

@dregad dregad left a comment

Choose a reason for hiding this comment

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

@CasN many thanks for your contribution. This looks good (mostly) and tested OK.

I'll make the necessary adjustments to fix my review comments and merge.

plugins/MantisGraph/MantisGraph.php Outdated
@@ -64,7 +64,7 @@ function register() {

$this->author = 'MantisBT Team';
$this->contact = 'mantisbt-dev@lists.sourceforge.net';
$this->url = 'https://mantisbt.org';
$this->url = 'https://www.mantisbt.org';
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing this, the URL is correct

plugins/MantisGraph/MantisGraph.php
@@ -227,7 +227,7 @@ private function get_url_with_filter( $p_page ) {
*/
function summary_menu() {
$t_menu_items[] = '<a href="'
. $this->get_url_with_filter( 'developer_graph.php' )
. $this->get_url_with_filter( 'project_graph.php' )
Copy link
Member

Choose a reason for hiding this comment

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

Changing the default graph is fine, but it needs to be reflected in all graph scripts

print_summary_menu( 'project_graph.php', $t_filter );

Otherwise the Graphs tab will no longer be marked as active.

plugins/MantisGraph/lang/strings_english.txt Outdated
@@ -88,3 +88,6 @@ $s_plugin_MantisGraph_graph_topdev = 'Top Developers by Fixed Issues';
$s_plugin_MantisGraph_graph_opendev = 'Developers by Open Issues';
$s_plugin_MantisGraph_graph_topreporter_fixed = 'Top Reporters by Fixed Issues';
$s_plugin_MantisGraph_other_categories = 'other categories (%d)';

$s_plugin_MantisGraph_other_projects = 'other projecst (%d)';
Copy link
Member

Choose a reason for hiding this comment

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

Typo

plugins/MantisGraph/lang/strings_english.txt Outdated
@@ -88,3 +88,6 @@ $s_plugin_MantisGraph_graph_topdev = 'Top Developers by Fixed Issues';
$s_plugin_MantisGraph_graph_opendev = 'Developers by Open Issues';
$s_plugin_MantisGraph_graph_topreporter_fixed = 'Top Reporters by Fixed Issues';
$s_plugin_MantisGraph_other_categories = 'other categories (%d)';

$s_plugin_MantisGraph_other_projects = 'other projecst (%d)';
$s_plugin_MantisGraph_graph_imp_project_title = 'By Projects Graphs';
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you're tagging these 2 strings at the end, it would be more consistent to put them together with related strings. Also the imp prefix is probably the result of a copy/paste, not needed IMO (TBH, I'm not even what its meaning is/was).

@dregad dregad merged commit c03fa0c into mantisbt:master Feb 18, 2024
1 check failed
@dregad
Copy link
Member

dregad commented Feb 19, 2024

Also, to keep in mind for future contributions:

When referring to Mantis issues in your commit message, you should always follow the regex from source integration plugin ((?:fixe?d?s?|resolved?s?)+\s*:?\s+(?:#(?:\d+)[,\.\s]*)+), to ensure that when it is merged

  • the commit is automatically linked to the issue(s)
  • the issue(s) is/are marked as resolved

In other words

  • DON'T: This addresses issue 33521
  • DO: Fix #33521, Fixes #33521 or Resolves #33521, etc.

You can reference multiple issues if needed, e.g. Fix #1, #2... and if you just want to link an issue without affecting the resolved status, you can use (?:bugs?|issues?|reports?)+\s*:?\s+(?:#(?:\d+)[,\.\s]*)+, i.e. Bug #1, Issues #2, #3 etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dregad dregad dregad approved these changes

Assignees

@dregad dregad

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@CasN @dregad