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

[Issue tracker] change userID to full name #8451

Merged
merged 8 commits into from
Mar 22, 2023

Conversation

regisoc
Copy link
Contributor

@regisoc regisoc commented Mar 16, 2023

Brief summary of changes

Change user IDs to user full names throughout the main pages.

Testing instructions (if applicable)

  1. Go to the issue tracker.
  2. Check the assignee and reporter columns in the main table (there should be user full names).
  3. Click on one issue to print the details.
  4. All information should contain the user full names (i.e top of the page, table, and history section).

@regisoc regisoc requested a review from racostas March 16, 2023 13:46
@regisoc regisoc self-assigned this Mar 16, 2023
@regisoc regisoc added this to the 25.0.0 milestone Mar 16, 2023
@kongtiaowang
Copy link
Contributor

@regisoc in this file issue_tracker/test/Issue_TrackerTest.php add [ 'assignee' => 'TestUser', ] after line 63, it will pass test.

@racostas
Copy link
Contributor

Hi @regisoc , let's add the changes to the CHANGELOG file for keeping track https://github.com/aces/Loris/blob/main/CHANGELOG.md

@regisoc regisoc mentioned this pull request Mar 17, 2023
@@ -429,7 +452,7 @@ class Edit extends \NDB_Page implements ETagCalculator

$issueID = intval($values['issueID']);

$issueValues['lastUpdatedBy'] = $user->getUsername();
$issueValues['lastUpdatedBy'] = $user->getUserName();
Copy link
Contributor

Choose a reason for hiding this comment

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

since it is assigned in line 483 I think this input in 455 is not longer necessary .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be because it is used in line 473 (update on existing issue case) and 477 (new issue case).
This needed to be the username (userID) for the issues table. The value is just changed for the full name after database insert/update (line 483).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, yes, it's a bit confusing the way the code is wrote but I think this precedes this PR. For example $historyValues is assigned in 470 the used in 486, passing the DB updated in between without using the value :)

The PR is fine !

Copy link
Contributor

@racostas racostas left a comment

Choose a reason for hiding this comment

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

One single comment. Aside of that looks good.

@racostas racostas self-requested a review March 17, 2023 19:22
Copy link
Contributor

@racostas racostas left a comment

Choose a reason for hiding this comment

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

LGTM.

@racostas racostas added the Passed manual tests PR has been successfully tested by at least one peer label Mar 17, 2023
@driusan driusan merged commit 7f3e587 into aces:main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants