Skip to content

Leonjoppi#16

Open
LeJonPPI wants to merge 13 commits intoTestLinkOpenSourceTRMS:testlink_1_9from
LeJonPPI:leonjoppi
Open

Leonjoppi#16
LeJonPPI wants to merge 13 commits intoTestLinkOpenSourceTRMS:testlink_1_9from
LeJonPPI:leonjoppi

Conversation

@LeJonPPI
Copy link
Copy Markdown

@LeJonPPI LeJonPPI commented Feb 22, 2016

This request contains the changes from ticket 7385 and 7386.
If there are any questions, feel free to ask: jordans.leon@gmx.de or via the mantis tickets.

@fmancardi
Copy link
Copy Markdown
Contributor

fmancardi commented Feb 22, 2016

hi
few important things

  1. if you use an existent file as starting point to create a new one
    (manageSubs.tpl) is completely wrong to leave wrong comments in the file

=> your merge request for these files will be rejected.
please

  1. is subscriptions are applicable just to requirements, manageSubs is a
    bad name.

  2. indent standard for Testlink is 2 spaces.

  3. brute force copy and paste is not accepted

This is the offending code
$subject = str_replace("%req_doc_id", $req_doc_id, $subject);
$subject = str_replace("%reqTitle", $reqTitle, $subject);
$body = lang_get('req_delete_subscribtion');
$body = str_replace("%reqTitle", $reqTitle, $body);
$body = str_replace("%modifier", $modifier, $body);

$body = str_replace("%scope", $scope, $body);

$subject = str_replace("%req_doc_id", $req_doc_id, $subject);
$subject = str_replace("%reqTitle", $reqTitle, $subject);
$body = lang_get('req_version_create_subscribtion');
$body = str_replace("%reqTitle", $reqTitle, $body);
$body = str_replace("%modifier", $modifier, $body);
$body = str_replace("%log_msg", $log_msg, $body);
$body = str_replace("%scope", $scope, $body);

that has to be converted in a simple method

  1. this method is rejected
    5.1 no magic number constants are accepted
    5.2 this code layout is rejected
    private function getStatusIdentifier($statusAbbr, $langStr=null) {
  • $fullStatusName = "";
  • switch($statusAbbr) {
  • case "D": $fullStatusName = lang_get("req_status_draft",$langStr); break;
  • case "R": $fullStatusName = lang_get("req_status_review",$langStr); break;
  • case "W": $fullStatusName = lang_get("req_status_rework",$langStr); break;
  • case "F": $fullStatusName = lang_get("req_status_finish",$langStr); break;
  • case "I": $fullStatusName = lang_get("req_status_implemented",$langStr);
    break;
  • case "V": $fullStatusName = lang_get("review_status_valid",$langStr);
    break;
  • case "N": $fullStatusName = lang_get("req_status_not_testable",$langStr);
    break;
  • case "O": $fullStatusName = lang_get("req_status_obsolete",$langStr);
    break;
  • }
  • return $fullStatusName;
  1. can you explain what do you mean for subbed ?
  2. naming convention not in line with project => rejected
    fk_req_id int(11) NOT NULL,
  • fk_user_id int(11) NOT NULL,
  • tproject_id int(11) NOT NULL,

You are writting code in an existent enviroment that has it's own rules,
you can not change it introducing
yours.

more code review to come

On Mon, Feb 22, 2016 at 3:11 PM, LeJonPPI notifications@github.com wrote:

This request contains the changes from ticket 7385 and 7386.
If there are any questions, feel free to ask: jordans.leon@gmx.de or via

the mantis tickets.

You can view, comment on, or merge this pull request online at:

#16
Commit Summary

  • Implementation of the email-alert on requirement state change.
  • adding the table creation to the install script and solving minor
    syntax error
  • Minor bugfixes.
  • log commands removed.
  • Merge branch 'testlink_1_9' of
    https://github.com/TestLinkOpenSourceTRMS/testlink-code into
    testlink_1_9
  • build the subscription mechanisms for requirements (see mantis id
    0007386)
  • notification for subbed users on requirement delete, change, version
    create and version delete
  • develp a new user interface to handle requirement subscribtions
  • removing import of custom_config.php

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#16.

Francisco Mancardi

@fmancardi
Copy link
Copy Markdown
Contributor

fmancardi commented Feb 22, 2016

More issues
$sql = "SELECT id, login, email"

  • ." FROM users, req_subscription"
  • ." WHERE req_subscription.fk_req_id = $requirement_id"
  • ." AND req_subscription.fk_user_id = users.id"
  • ." AND req_subscription.tproject_id = $tproject_id;";
  • $results = $this->db->get_recordset($sql);
  • return $results;
    This will not work if people use table prefix during testlink installation.
    Rejected

On Mon, Feb 22, 2016 at 10:19 PM, Francisco Mancardi <
francisco.mancardi@gmail.com> wrote:

hi
few important things

  1. if you use an existent file as starting point to create a new one
    (manageSubs.tpl) is completely wrong to leave wrong comments in the file

=> your merge request for these files will be rejected.
please

  1. is subscriptions are applicable just to requirements, manageSubs is a
    bad name.

  2. indent standard for Testlink is 2 spaces.

  3. brute force copy and paste is not accepted

This is the offending code
$subject = str_replace("%req_doc_id", $req_doc_id, $subject);
$subject = str_replace("%reqTitle", $reqTitle, $subject);
$body = lang_get('req_delete_subscribtion');
$body = str_replace("%reqTitle", $reqTitle, $body);
$body = str_replace("%modifier", $modifier, $body);

$body = str_replace("%scope", $scope, $body);

$subject = str_replace("%req_doc_id", $req_doc_id, $subject);
$subject = str_replace("%reqTitle", $reqTitle, $subject);
$body = lang_get('req_version_create_subscribtion');
$body = str_replace("%reqTitle", $reqTitle, $body);
$body = str_replace("%modifier", $modifier, $body);
$body = str_replace("%log_msg", $log_msg, $body);
$body = str_replace("%scope", $scope, $body);

that has to be converted in a simple method

  1. this method is rejected
    5.1 no magic number constants are accepted
    5.2 this code layout is rejected
    private function getStatusIdentifier($statusAbbr, $langStr=null) {
  • $fullStatusName = "";
  • switch($statusAbbr) {
  • case "D": $fullStatusName = lang_get("req_status_draft",$langStr); break
    ;
  • case "R": $fullStatusName = lang_get("req_status_review",$langStr);
    break;
  • case "W": $fullStatusName = lang_get("req_status_rework",$langStr);
    break;
  • case "F": $fullStatusName = lang_get("req_status_finish",$langStr);
    break;
  • case "I": $fullStatusName = lang_get("req_status_implemented",$langStr);
    break;
  • case "V": $fullStatusName = lang_get("review_status_valid",$langStr);
    break;
  • case "N": $fullStatusName = lang_get("req_status_not_testable",$langStr);
    break;
  • case "O": $fullStatusName = lang_get("req_status_obsolete",$langStr);
    break;
  • }
  • return $fullStatusName;
  1. can you explain what do you mean for subbed ?
  2. naming convention not in line with project => rejected
    fk_req_id int(11) NOT NULL,
  • fk_user_id int(11) NOT NULL,
  • tproject_id int(11) NOT NULL,

You are writting code in an existent enviroment that has it's own rules,
you can not change it introducing
yours.

more code review to come

On Mon, Feb 22, 2016 at 3:11 PM, LeJonPPI notifications@github.com
wrote:

This request contains the changes from ticket 7385 and 7386.
If there are any questions, feel free to ask: jordans.leon@gmx.de or via

the mantis tickets.

You can view, comment on, or merge this pull request online at:

#16
Commit Summary

  • Implementation of the email-alert on requirement state change.
  • adding the table creation to the install script and solving minor
    syntax error
  • Minor bugfixes.
  • log commands removed.
  • Merge branch 'testlink_1_9' of
    https://github.com/TestLinkOpenSourceTRMS/testlink-code into
    testlink_1_9
  • build the subscription mechanisms for requirements (see mantis id
    0007386)
  • notification for subbed users on requirement delete, change,
    version create and version delete
  • develp a new user interface to handle requirement subscribtions
  • removing import of custom_config.php

File Changes

Patch Links:

https://github.com/TestLinkOpenSourceTRMS/testlink-code/pull/16.patch

—
Reply to this email directly or view it on GitHub
#16.

Francisco Mancardi

Francisco Mancardi

@neiesc
Copy link
Copy Markdown
Contributor

neiesc commented May 4, 2025

Up @fmancardi

@fmancardi
Copy link
Copy Markdown
Contributor

fmancardi commented May 4, 2025

No plans to integrate

@neiesc
Copy link
Copy Markdown
Contributor

neiesc commented May 4, 2025

@fmancardi ok maybe close this?

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LeJonPPI @fmancardi @neiesc