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

[issue_tracker] Fix bug that prevents new issue creation #9643

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[issue_tracker] Fix bug that prevents new issue creation #9643

wants to merge 6 commits into from

Conversation

shonibare
Copy link
Contributor

Related to #9606

  • This PR fixes the issue description. The cause of the issue is the change of CandID => CandidateID.
  • It also resolves a bug that causes the Edit Issue page to fail to display when an attachment is included in an issue.

To Test:

  • See if you can create a new issue or update existing issue
  • Upload a file to an issue and see if the page doesn't break and you can see the attachments/file when the page refreshes.

@shonibare shonibare added Priority: High PR or issue should be prioritised over others for review and testing Module: issue_tracker PR or issue related to issue tracker module 27.0.0 - Bugs Bugs Found in LORIS 27 testing labels Feb 26, 2025
@shonibare shonibare added Language: SQL PR or issue that update SQL code Language: PHP PR or issue that update PHP code labels Feb 26, 2025
driusan
driusan reviewed Feb 28, 2025
View reviewed changes
SQL/0000-00-00-schema.sql
@@ -1564,7 +1564,7 @@ CREATE TABLE `issues_history` (
`issueHistoryID` int(11) unsigned NOT NULL AUTO_INCREMENT,
`newValue` longtext NOT NULL,
`dateAdded` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,
`fieldChanged` enum('assignee','status','comment','sessionID','centerID','title','category','module','lastUpdatedBy','priority','candID', 'description','watching','instrument') NOT NULL DEFAULT 'comment',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is right. The CandID is the field that should be exposed to the user. The sequential CandidateID should not be leaked to the frontend anywhere, but only used for joins.

Copy link
Contributor

Choose a reason for hiding this comment

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

After investigating, I think this is actually fine. As long as it doesn't get brought to the front-end (which it does), but it isn't because of its presence in this enum.

jeffersoncasimir
jeffersoncasimir requested changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The following issue was not necessarily introduced by this PR.

I don't think CandidateID should be brought to the front end.

There are a few locations where SELECT issues.* (or i.*) is used, instead of unsetting or omitting CandidateID, and candidate.CandID is also being brought to the front-end, without seemingly being used.

The payloads to the front-end should definitely not include CandidateID; present in at least both Browse and Edit.

After a thorough investigation, I am otherwise satisfied with how things are done.

@shonibare
Copy link
Contributor Author

@jeffersoncasimir did you try to find the source of this bug #9606. From which table is this PSCID being called/stored? The newValue in the issue_history is responsible for the PSCID in the Comment & History section. Meanwhile, this is the output of this PR. The CandidateID is not exposed in the front-end/links

Screenshot 2025-02-28 at 3 56 19 PM Screenshot 2025-02-28 at 3 56 39 PM

The CandID is replaced with CandidateID in the issues tbl https://github.com/jeffersoncasimir/Loris/blob/e37ad6e670a415a2b8687c4b34449afcf6269cec/SQL/New_patches/2025_02_05_change_candid_fk_to_id.sql#L3

@shonibare
Copy link
Contributor Author

@jeffersoncasimir I have successfully hidden the CandID & CandidateID from the front end as you suggested and nothing breaks. Let me know if it looks good.
Screenshot 2025-02-28 at 6 16 15 PM

jeffersoncasimir
jeffersoncasimir approved these changes Mar 3, 2025
View reviewed changes
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir left a comment

Choose a reason for hiding this comment

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

Looks great! Tested and approved!

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

@driusan driusan driusan left review comments

@jeffersoncasimir jeffersoncasimir jeffersoncasimir approved these changes

Assignees

@jeffersoncasimir jeffersoncasimir

Labels
27.0.0 - Bugs Bugs Found in LORIS 27 testing Language: PHP PR or issue that update PHP code Language: SQL PR or issue that update SQL code Module: issue_tracker PR or issue related to issue tracker module Priority: High PR or issue should be prioritised over others for review and testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shonibare @driusan @jeffersoncasimir