Loris: Add a GitHub issue for every TODO, FIXME, and XXX comment in the codebase

Created on 19 Nov 2019 · 17Comments · Source: aces/Loris

It's rare that code annotated with TODOs, FIXMEs, etc. actually get fixed. Someone has to stumble across them by chance and would have to put what they're doing on pause to fix that issue.

We also have GitHub as a public issue tracker so any refactoring tasks should be tracked here.

TODO

Adding the following lines to docs/LorisCS.xml will enable PHPCS to flag TODOs and FIXMEs. (There is no sniff for XXX annotations but these should also be transferred to Issues.)

<rule ref="Generic.Commenting.Fixme"/>
<rule ref="Generic.Commenting.Todo"/>

Then run make checkstatic. The PHPCS section of its report should be transcribed into GitHub issues.

Once the issues are created, the modified file docs/LorisCS.xml can be committed to the repo to prevent future TODOs from being committed to the code.

Beginner Friendly Cleanup

Source

johnsaigle

All 17 comments

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] on 18 Jan 2020

FILE: /var/www/loris/php/libraries/File_Upload.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

| | return an error"

FILE: /var/www/loris/php/libraries/NDB_Page.class.inc

FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 6 LINES

685 | WARNING | Comment refers to a FIXME task "For historical compatibility with NDB_Menu_Filter"

FILE: /var/www/loris/php/libraries/NDB_Menu_Filter.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

| | setTemplateVar"

FILE: /var/www/loris/php/libraries/NDB_BVL_Feedback.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

| | I'm not sure"

FILE: /var/www/loris/php/libraries/SinglePointLogin.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

58 | WARNING | Comment refers to a FIXME task "This is a temporary hack until the password reset

| | logic is"

FILE: /var/www/loris/php/libraries/Module.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

84 | WARNING | Comment refers to a TODO task "Module search path should be a config option"
314 | WARNING | Comment refers to a FIXME task "Hack required for breadcrumbs. This should be

| | removed,"

FILE: /var/www/loris/php/libraries/NDB_BVL_Instrument.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

582 | WARNING | Comment refers to a FIXME task "Why is this here? The body of this if"

FILE: /var/www/loris/php/libraries/Database.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

351 | WARNING | Comment refers to a FIXME task "this check is only here because of a quirk of the"
951 | WARNING | Comment refers to a FIXME task "This function does not have a return type

| | declaration because"

FILE: /var/www/loris/php/libraries/Project.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

| | Project table"

FILE: /var/www/loris/php/libraries/Candidate.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

180 | WARNING | Comment refers to a FIXME task "Ideally the signature of this function should be

| | changed so that"

FILE: /var/www/loris/php/libraries/NDB_BVL_Battery.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

239 | WARNING | Comment refers to a TODO task

FILE: /var/www/loris/php/libraries/NDB_Notifier.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

83 | WARNING | Comment refers to a TODO task "add validation for email sent/failed"

95 | WARNING | Comment refers to a TODO task "send confirmation email instead"

FILE: /var/www/loris/php/libraries/NDB_BVL_Instrument_LINST.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

27 | WARNING | Comment refers to a FIXME task "LINST instruments should parse the full name"

598 | WARNING | Comment refers to a FIXME task "This variable is never populated"

FILE: /var/www/loris/php/libraries/NDB_Config.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

| | null"

FILE: /var/www/loris/php/libraries/LorisForm.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

1481 | WARNING | Comment refers to a TODO task "Implement this in *HTML functions"

FILE: /var/www/loris/htdocs/router.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

24 | WARNING | Comment refers to a FIXME task "Should this be in the main index.php to prevent the

| | need"

FILE: /var/www/loris/htdocs/index.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

25 | WARNING | Comment refers to a FIXME task "The code in NDB_Client should mostly be replaced by

| | middleware"

FILE: /var/www/loris/modules/conflict_resolver/test/conflict_resolverTest.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

272 | WARNING | Comment refers to a TODO task "find this"

FILE: /var/www/loris/modules/login/php/requestaccount.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

86 | WARNING | Comment refers to a FIXME task "These error checks are direct ports from when the

| | request"

FILE: /var/www/loris/modules/instrument_list/php/instrument_list.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

116 | WARNING | Comment refers to a FIXME task "The longterm solution is to remove the

| | getControlPanel call from the"

FILE: /var/www/loris/modules/user_accounts/test/user_accountsTest.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

252 | WARNING | Comment refers to a TODO task "add test case to ensure pending_approval"

FILE: /var/www/loris/modules/user_accounts/php/edit_user.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

1110 | WARNING | Comment refers to a TODO task "This validation should be done on the front-end

| | instead"

FILE: /var/www/loris/modules/user_accounts/php/my_preferences.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

400 | WARNING | Comment refers to a TODO task "This validation should be done on the front-end

| | instead"

FILE: /var/www/loris/modules/api/php/endpoints/candidate/visit/visit.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

166 | WARNING | Comment refers to a TODO task "There is no way to validate the the visit_label in

| | the url"

FILE: /var/www/loris/modules/api/php/endpoints/candidate/visit/image/qc.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

173 | WARNING | Comment refers to a TODO task "This is (and was) not checking or handling Caveats"

FILE: /var/www/loris/modules/api/php/endpoints/candidate/visit/instrument/flags.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

99 | WARNING | Comment refers to a TODO task "I don`t think this was working in v0.0.3-dev"

FILE: /var/www/loris/modules/api/php/endpoints/candidate/visit/instrument/instrument.class.inc

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES

155 | WARNING | Comment refers to a TODO task "Check permissions. How??"

193 | WARNING | Comment refers to a TODO task "Check permissions. How??"

FILE: /var/www/loris/modules/api/php/endpoints/candidate/candidate.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

134 | WARNING | Comment refers to a TODO task "User permission to acces this and subendpoints"

FILE: /var/www/loris/modules/api/php/endpoints/project/visits.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

84 | WARNING | Comment refers to a FIXME task "Validate project based permissions"

FILE: /var/www/loris/modules/api/php/views/project.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

101 | WARNING | Comment refers to a TODO task "This should be replace by

| | $this->_project->getVisitLabels();"

FILE: /var/www/loris/modules/api/php/views/visit/image/format/brainbrowser.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

42 | WARNING | Comment refers to a TODO task "Can time dimention always be included?"

FILE: /var/www/loris/modules/timepoint_list/php/timepoint_list.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

88 | WARNING | Comment refers to a FIXME task "Update the code to add the singleton here, not the

| | identifier"

FILE: /var/www/loris/modules/timepoint_list/php/module.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

43 | WARNING | Comment refers to a FIXME task "This should be reworked to not silently handle the

| | try-catch"

FILE: /var/www/loris/modules/dataquery/ajax/queryNotEqual.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

47 | WARNING | Comment refers to a TODO task "Rewrite this using array_filter and array_map"

FILE: /var/www/loris/modules/genomic_browser/ajax/genomic_file_upload.php

FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES

407 | WARNING | Comment refers to a TODO task "validate probe value"

FILE: /var/www/loris/modules/imaging_qc/php/imaging_qc.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

42 | WARNING | Comment refers to a FIXME task "This module should have its own permission"

FILE: /var/www/loris/modules/bvl_feedback/test/BvlFeedbackTest.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

63 | WARNING | Comment refers to a TODO task "Any instrument"

FILE: /var/www/loris/modules/bvl_feedback/ajax/close_bvl_feedback_thread.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

41 | WARNING | Comment refers to a FIXME task "This allows to close any thread as long as the

| | feedbackid"

FILE: /var/www/loris/modules/examiner/php/editexaminer.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

138 | WARNING | Comment refers to a FIXME task "$visit_label should be set properly. This

| | declaration"

FILE: /var/www/loris/modules/document_repository/php/uploadcategory.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

88 | WARNING | Comment refers to a TODO task "check duplicate name category"

FILE: /var/www/loris/modules/new_profile/php/new_profile.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

78 | WARNING | Comment refers to a TODO task "add similar logic to projects when user-projects

| | association is"

FILE: /var/www/loris/modules/data_release/ajax/AddPermission.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

8 | WARNING | Comment refers to a TODO task

FILE: /var/www/loris/modules/survey_accounts/test/survey_accountsTest.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

258 | WARNING | Comment refers to a TODO task "add a survey successful"

FILE: /var/www/loris/modules/statistics/php/statistics_mri_site.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

217 | WARNING | Comment refers to a FIXME task "The function _completeCount() seems to be

| | unfinished and"

FILE: /var/www/loris/modules/statistics/php/statistics_site.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

43 | WARNING | Comment refers to a TODO task "Create a permission specific to statistics"

FILE: /var/www/loris/modules/instruments/php/module.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

54 | WARNING | Comment refers to a TODO task "These magic numbers should be changed to constants so

| | that"

FILE: /var/www/loris/modules/issue_tracker/php/issue.class.inc

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

136 | WARNING | Comment refers to a FIXME task "This should negotiate the content type it returns

| | (JSON vs HTML"

FILE: /var/www/loris/modules/issue_tracker/ajax/EditIssue.php

FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES

31 | WARNING | Comment refers to a TODO task "or split it into two files... :P"
41 | WARNING | Comment refers to a TODO task "encapsulate more"

749 | WARNING | Comment refers to a TODO task "this is dumb"

FILE: /var/www/loris/modules/configuration/ajax/process.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

6 | WARNING | Comment refers to a FIXME task "This code should be refactored away from using the

| | separate 'ajax'"

johnsaigle on 29 Jan 2020

The Stale label is being removed automatically because some activity has occurred or because the developers have decided that this pull request is important and should not continue to be overlooked.

stale[bot] on 29 Jan 2020

Hello, new contributor here! I will be happy to lend a hand to solve this task, as it will give me a good overview of the project.

laemtl on 26 Mar 2020

👍1

Hey, that's great! We appreciate your contribution. let us know if you have any questions. :)

johnsaigle on 30 Mar 2020

@johnsaigle -- Is this a beginner task truly? I suspect a lot of the fixme and todo comments often need interpretation by an experienced Loris dev to define/create the underlying issue scope.

@laemtl I'll assign this to you for now as we wait for Testing assignments. This should be done on the master branch since discussion may be required - i.e. may take time to resolve.

christinerogers on 6 May 2020

Is this a beginner task truly?

I think that creating an issue that includes a link to the relevant section of code using a GitHub permalink is a beginner-friendly task. Devs converting TODOs to GitHub Issues aren't expected to necessarily resolve the issue to which the tickets refer. Yes, many will likely need subsequent attention from someone more experienced. This would be a good way to spotlight these problems.

johnsaigle on 6 May 2020

I wrote a small script to automate the creation of the issues.

laemtl on 10 May 2020

TODOs should absolutely not be removed from the code. That's like "fixing bugs" by removing assertions. If the author thought it was important enough to note a limitation of the code someone who's reading or debugging it shouldn't need a third party service and access to know what it is.

driusan on 11 May 2020

@johnsaigle - for discussion or close this issue? I think you tried to bring it up at a Loris meeting months ago? I agree with Dave otherwise...

christinerogers on 11 May 2020

I'll mark it for discussion and add to the agenda.

johnsaigle on 11 May 2020

The result of the discussion was to keep the TODOs in the code but to also track them using GitHub Issues. I'll change the issue title to match.

johnsaigle on 13 May 2020

👍1

the next few month could be a good time to really look into these issues and discuss with dave what should be done @laemtl

christinerogers on 16 Jun 2020

As I said above, I wrote a script which automate the creation of issues based on keywords found in the codebase. The downside is that more than hundreds of issues will be created at once.

laemtl on 16 Jun 2020

Right, and it was agreed that a huge influx of these issues wasn't desirable. So let's find a way to triage this into manageable parts.
One place to start is by really looking into a few of these issues and discussing with Dave what should be done on a few examples, and then extrapolating from there to triage these comments by what should be prioritized for Issue/code cleanup.
For example, maybe it makes sense to target the core php libraries first -- or maybe it doesn't because (hypothetically) of upcoming architecture changes.

christinerogers on 17 Jun 2020

@christinerogers Good idea! Targeting TODO for the Core outputs 18 results:

./tools/exporters/dataExtractor.php: // TODO This should likely be refactored at some point.
./tools/detect_duplicated_commentids.php: * TODO:
./test/unittests/NDB_PageTest.php: * TODO: Update this test once addCheckbox method is fixed.
./test/unittests/BreadcrumbTrailTest.php: * TODO: Add potential edge cases (such as white space)
./test/unittests/UtilityTest.php: * TODO Potential edge cases: test with the study_site and DCC booleans as false
./test/unittests/UtilityTest.php: * TODO Potential edge cases: Set 'Active' to 'N'
./test/unittests/NDB_Menu_Filter_Test.php: * TODO:
./test/unittests/BreadcrumbTest.php: * TODO: Add potential edge cases (such as white space)
./test/unittests/CandidateTest.php: * TODO This function calls Site::singleton() and User::singleton()
./php/libraries/LorisForm.class.inc: * TODO: Implement this in *HTML functions.
./php/libraries/NDB_Notifier.class.inc: * TODO add validation for email sent/failed (#6488)
./php/libraries/NDB_Notifier.class.inc: // TODO send confirmation email instead
./php/libraries/Project.class.inc: // TODO :: Add not null constraint to the name column of the Project table (#6479)
./php/libraries/Project.class.inc: // TODO :: Add unique constraint to the name column of the Project table (#6479)
./php/libraries/Module.class.inc: // TODO: Module search path should be a config option. (#6496)
./php/libraries/File_Upload.class.inc: //@todo Should test to see if form is valid and if not return an error. (#6490)
./php/libraries/NDB_BVL_Battery.class.inc: * @todo make it do something
./test/README.md:## Todo

I suggest to sightly edit all these comments to add a reference to the issue, like this:
// TODO (#7777):
This provides the double benefits of being able to distinguish the ones not entered in the system yet, and to link to the created issue from the code.
We can also remove the keyword TODO (?) from the comment if we want to turn on the rule in PHPCS after this issue has been processed.

@driusan Can it be a good starting point?

laemtl on 17 Jun 2020

👍1

Let's pause on this until we're through the current testing cycle. We'll re-discuss in November.

christinerogers on 15 Sep 2020

Was this page helpful?

0 / 5 - 0 ratings

Related issues

[mri violations] MRI Violations module does not describe or test widgets

driusan · 3Comments

[candidate_parameters] extra slash after clicking Candidate Info

maltheism · 4Comments

[documentation][install centOS] remove reference to bugfix branch

maltheism · 5Comments

[Dicom Archive] Clicking on the Archive Location link always results in a 404

HenriRabalais · 6Comments

[Issue tracker] Create new issue console error

zaliqarosli · 4Comments