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

[Schedule Module] add new schedule module into Loris #6150

Merged
merged 168 commits into from
Apr 15, 2024
Merged

[Schedule Module] add new schedule module into Loris #6150

merged 168 commits into from
Apr 15, 2024

Conversation

kongtiaowang
Copy link
Contributor

@kongtiaowang kongtiaowang commented Mar 9, 2020 •
edited

Brief summary of changes

Rewrite this module from IBIS.

Testing instructions

Test plan
Screen Shot 2020-03-10 at 1 15 34 PM

@kongtiaowang kongtiaowang added Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Feature PR or issue introducing/requiring at least one new feature Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Mar 9, 2020
@johnsaigle johnsaigle added the SQL PR contains SQL modifications such as schema changes and new SQL scripts label Mar 10, 2020
johnsaigle
johnsaigle previously requested changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

Small review. Some of the code needs changing to meet the core LORIS standards.

I'm not really sure what's going on with the new classes being added that deal with SQL statements. It seems unusual for LORIS

modules/schedule_module/php/appointment.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/appointment.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/appointment.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/appointment.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/appointment.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/appointment.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/data_entry.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/data_entry.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/data_entry.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/data_entry.class.inc Outdated Show resolved Hide resolved
@kongtiaowang
Copy link
Contributor Author

@johnsaigle Thanks for your reviews.

@kongtiaowang
Copy link
Contributor Author

restart travis

@kongtiaowang kongtiaowang reopened this Mar 11, 2020
@kongtiaowang kongtiaowang added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Apr 6, 2020
@kongtiaowang
Copy link
Contributor Author

This PR can't start Travis. I will reopen it.

@kongtiaowang kongtiaowang reopened this Apr 24, 2020
@kongtiaowang
Copy link
Contributor Author

restart travis

@kongtiaowang kongtiaowang reopened this Apr 24, 2020
@kongtiaowang kongtiaowang removed Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Jun 2, 2020
@kongtiaowang
Copy link
Contributor Author

@zaliqarosli Could you test this module?

zaliqarosli
zaliqarosli reviewed Jul 20, 2020
View reviewed changes
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

this all looks really good! there's just a space missing in the sql patch, and some php lines may still fail travis

i will approve review once they're resolved!

SQL/New_patches/2020-03-03-schedule_module.sql Outdated Show resolved Hide resolved
modules/schedule_module/php/modulerow.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/php/modulerow.class.inc Outdated Show resolved Hide resolved
zaliqarosli
zaliqarosli requested changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

re-review after testing the front end

modules/schedule_module/jsx/scheduleIndex.js Outdated Show resolved Hide resolved
modules/schedule_module/jsx/scheduleIndex.js Outdated Show resolved Hide resolved
modules/schedule_module/jsx/scheduleIndex.js Outdated Show resolved Hide resolved
modules/schedule_module/jsx/scheduleIndex.js Outdated Show resolved Hide resolved
modules/schedule_module/php/schedulemoduleprovisioner.class.inc Outdated Show resolved Hide resolved
modules/schedule_module/jsx/scheduleIndex.js Outdated Show resolved Hide resolved
modules/schedule_module/jsx/scheduleIndex.js Show resolved Hide resolved
modules/schedule_module/jsx/scheduleIndex.js Outdated Show resolved Hide resolved
modules/schedule_module/jsx/scheduleIndex.js Outdated Show resolved Hide resolved
@kongtiaowang kongtiaowang changed the base branch from master to main July 23, 2020 18:23
@kongtiaowang kongtiaowang removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Dec 20, 2023
@GeorgeMurad GeorgeMurad self-requested a review February 28, 2024 19:12
@GeorgeMurad
Copy link
Contributor

@kongtiaowang I have tested the PR and everything looks good except for one thing:

  • Getting 500 error when adding values to the DCCID input in the Add Appointment section. Whereas it's working fine when adding values to the PSCID input. Automatically the DCCID will be added.

@kongtiaowang kongtiaowang added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 4, 2024
@kongtiaowang kongtiaowang removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 7, 2024
@GeorgeMurad
Copy link
Contributor

All looks good but on the Add Appointment the visit is not shown

@kongtiaowang
Copy link
Contributor Author

@GeorgeMurad visit is good now.

GeorgeMurad
GeorgeMurad approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@GeorgeMurad GeorgeMurad left a comment

Choose a reason for hiding this comment

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

LGTM

@driusan driusan merged commit 791b857 into aces:main Apr 15, 2024
28 checks passed
@driusan driusan mentioned this pull request Apr 15, 2024
Closed
driusan added a commit to driusan/Loris that referenced this pull request Apr 15, 2024
@driusan
[Test] Check if tests are still working.
c4d81c0 
This is a test to see if merging aces#6150 caused
tests to fail. All tests were passing on the PR,
but it was an old enough PR that it's possible that
the static check rules were changed in the interm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CamilleBeau CamilleBeau CamilleBeau approved these changes

@GeorgeMurad GeorgeMurad GeorgeMurad approved these changes

@johnsaigle johnsaigle johnsaigle left review comments

@zaliqarosli zaliqarosli zaliqarosli left review comments

@ridz1208 ridz1208 Awaiting requested review from ridz1208

@maltheism maltheism Awaiting requested review from maltheism

@driusan driusan Awaiting requested review from driusan

Assignees

@kongtiaowang kongtiaowang

Labels
Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Feature PR or issue introducing/requiring at least one new feature Passed Manual Tests PR has undergone proper testing by at least one peer SQL PR contains SQL modifications such as schema changes and new SQL scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet