Skip to content

[issue_tracker] Add Instrument dropdown to Issue form #9311

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
Merged
driusan merged 5 commits into from
Nov 6, 2024
Merged

[issue_tracker] Add Instrument dropdown to Issue form #9311

driusan merged 5 commits into from
Nov 6, 2024

Conversation

@ay-bh
Copy link
Contributor

@ay-bh ay-bh commented Jul 23, 2024

Brief summary of changes

  • Added 'Instrument' dropdown field to Issue Tracker

  • Updated database schema to include 'instrument' column in 'issues' table

  • Modified Issue Form to include new Instrument dropdown

  • Updated relevant PHP and JavaScript files to handle the new field

  • Have you updated related documentation?

Testing instructions (if applicable)

  1. Navigate to the Issue Tracker
  2. Create a new issue and verify that the 'Instrument' dropdown is present with a list of all instruments
  3. Select an instrument and save the issue
  4. Edit an existing issue and confirm the 'Instrument' field can be modified
  5. Verify that the instrument selection is saved correctly and displayed in the issue details

Link(s) to related issue(s)

@ay-bh ay-bh requested a review from racostas July 23, 2024 17:52
@racostas racostas self-assigned this Jul 25, 2024
@racostas racostas added the Event: GSOC PR or issue accepted for Google Summer of Code label Jul 29, 2024
racostas
racostas approved these changes Jul 29, 2024
View reviewed changes
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 Jul 29, 2024
@racostas
Copy link
Contributor

racostas commented Jul 29, 2024

This one is looking good to me. I leave you the final review @driusan. Thanks.

Thank you @ay-bh, great work !!

driusan
driusan previously requested changes Aug 12, 2024
View reviewed changes
SQL/0000-00-00-schema.sql Outdated
`candID` int(6) DEFAULT NULL,
`category` varchar(255) DEFAULT NULL,
`description` longtext DEFAULT NULL,
`instrument` varchar(255) DEFAULT NULL,
Copy link
Collaborator

@driusan driusan Aug 12, 2024

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 should be a varchar(255). The test_names table should have all instruments and this can/should be a foreign key.

Copy link
Contributor Author

@ay-bh ay-bh Sep 21, 2024

Choose a reason for hiding this comment

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

@driusan Please check now

Copy link
Collaborator

@driusan driusan Oct 29, 2024

Choose a reason for hiding this comment

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

Did you forget to push something? It is still a varchar(255). It should be an int(10) unsigned that references Test_names(ID)

Copy link
Contributor Author

@ay-bh ay-bh Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@driusan Apologies, I misunderstood. Request you to review now.

@christinerogers
Copy link
Contributor

christinerogers commented Oct 21, 2024

changes seem to have been addressed. bumping this for review -- @racostas @driusan
Thanks - We have a limited time window to get this merged as GSOC is ending in early November.

@christinerogers christinerogers dismissed driusan’s stale review October 25, 2024 16:54

changes seem to have been addressed. bumping for dave's re-review

@racostas
Copy link
Contributor

racostas commented Oct 29, 2024

Hi @ay-bh, you're in the good direction
just some formatting issues.
image

@ay-bh
Copy link
Contributor Author

ay-bh commented Oct 29, 2024

Hi @ay-bh, you're in the good direction just some formatting issues. image

@racostas I don't understand, I haven't modified this file at all.

@driusan
Copy link
Collaborator

driusan commented Oct 30, 2024

@ay-bh There was a problem where the main branch was failing which is now fixed, can you rebase?
@racostas can you re-test now that the testID change is done?

@ay-bh ay-bh force-pushed the add-instruments-issue-tracker branch from d1c799a to 1d99808 Compare October 30, 2024 15:08
racostas
racostas requested changes Nov 1, 2024
View reviewed changes
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.

Hi @ay-bh , two minor changes in the New_patches file, that we might want to include in the RB file as well. Please take a look at the comments in the corresponding file.

The rest is looking great!
image

image

Also I'm aware you're in a national festivities until Nov 4-5 so don't rush to solve this one. We can do it when you back. It's very small.

@racostas racostas added State: Needs work PR awaiting additional work by the author to proceed and removed Passed manual tests PR has been successfully tested by at least one peer labels Nov 1, 2024
ay-bh and others added 2 commits November 2, 2024 06:55
@ay-bh @racostas
Update SQL patch to include watching
71d2f59 
Co-authored-by: racostas <37309344+racostas@users.noreply.github.com>
@ay-bh @racostas
Update AFTER field in SQL patch
5319501 
Co-authored-by: racostas <37309344+racostas@users.noreply.github.com>
@ay-bh ay-bh requested a review from racostas November 2, 2024 14:53
racostas
racostas approved these changes Nov 3, 2024
View reviewed changes
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.

All tested again. LGTM !

@racostas racostas added Passed manual tests PR has been successfully tested by at least one peer and removed State: Needs work PR awaiting additional work by the author to proceed labels Nov 3, 2024
@racostas
Copy link
Contributor

racostas commented Nov 4, 2024

This one looks good @driusan , I left you the final review. Thanks !

@driusan driusan merged commit 04450c4 into aces:main Nov 6, 2024
10 checks passed
ZhichGaming pushed a commit to ZhichGaming/Loris that referenced this pull request Nov 25, 2024
@ay-bh @ZhichGaming
[issue_tracker] Add Instrument dropdown to Issue form (aces#9311)
232bdbe 
## Brief summary of changes

Added 'Instrument' dropdown field to Issue Tracker

* Resolves aces#8896
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@racostas racostas racostas approved these changes

@driusan driusan Awaiting requested review from driusan

Assignees

@racostas racostas

Labels

Event: GSOC PR or issue accepted for Google Summer of Code Passed manual tests PR has been successfully tested by at least one peer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Issue Tracker] Add ‘Instrument’ from a drop down of all instruments

4 participants

@ay-bh @racostas @christinerogers @driusan