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

Integration of VIT-AP Campus #247

Merged
merged 13 commits into from
Jun 28, 2024
Merged

Conversation

anantgupta001
Copy link
Contributor

Overview

This pull request addresses Issue #124 by integrating the VIT-AP campus into the existing codebase for VIT Vellore and Chennai campuses. The modifications span across multiple files, ensuring comprehensive support for the new campus.

Files Modified

  1. timetable.js: Added logic to handle VIT-AP specific timetable structures.
  2. schema.js: Updated schema to accommodate VIT-AP data models.
  3. convert_xlsx_to_json.js: Enhanced to process VIT-AP Excel files.
  4. convert_json_to_data.js: Adapted to include VIT-AP related data conversions.
  5. index.html: Integrated VIT-AP campus options in the user interface.
  6. main.js: Incorporated VIT-AP specific functionalities and features.

Issue

Currently, we do not have an Excel sheet for VIT-AP campus data. Therefore, while the code is prepared to handle VIT-AP data, testing and full validation remain pending until the necessary data file is provided.

Request

Please review the changes and approve the integration. Additionally, once the VIT-AP Excel sheet is available, we can conduct thorough testing to ensure all functionalities work as expected.

Conclusion

The integration of VIT-AP campus has been meticulously implemented across the relevant files. Pending the provision of the required Excel sheet, we are ready to proceed with final testing and validation.

Thank you for your attention and cooperation.

@vatz88
Copy link
Owner

vatz88 commented Jun 26, 2024

Thanks for the PR @anantgupta001 !

I'd recommend we wait until we have the data. And once we have it, we can should fully test this before merging.

@vatz88
Copy link
Owner

vatz88 commented Jun 26, 2024

@therealsujitk if you get time, please do review these changes so we're ready merge when data is available

@vatz88 vatz88 requested a review from therealsujitk June 26, 2024 19:39
@therealsujitk
Copy link
Collaborator

Sure @vatz88, I've mentioned what needs to be done in the related issue (#124).

@therealsujitk
Copy link
Collaborator

The changes have passed all necessary tests and can be used for testing purposes.

There are no tests for AP related files, can you add these here --> https://github.com/vatz88/FFCSonTheGo/blob/master/tests/xlsx.test.js

@anantgupta001
Copy link
Contributor Author

@therealsujitk all the given tasks has been completed, please do let me know if anything required.

tests/xlsx.test.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/js/main.js Outdated Show resolved Hide resolved
src/js/timetable.js Outdated Show resolved Hide resolved
@therealsujitk therealsujitk linked an issue Jun 28, 2024 that may be closed by this pull request
@therealsujitk
Copy link
Collaborator

Also, could you resolve the conflict in package.json? Thanks.

src/index.html Outdated Show resolved Hide resolved
@anantgupta001
Copy link
Contributor Author

Schema test is not working.
image
please help me resolving this.

@therealsujitk
Copy link
Collaborator

Schema test is not working.

This test means that these slots present in the course .xlsx list are not present in the timetable schema. If you have the latest timetable for the AP campus you can update the schema JSON file, if not we'll have to skip this test for now.

@therealsujitk
Copy link
Collaborator

@anantgupta001 do you not have the latest timetable? It's better if we fix it before merging, I only said we could skip it if it was absolutely not possible to fix right now.

@anantgupta001
Copy link
Contributor Author

@therealsujitk, I currently do not have the latest timetable. However, in some time, we will have our Fall semester 2024-25 registration. Once that occurs, I will update the information accordingly.

Copy link
Collaborator

@therealsujitk therealsujitk left a comment

Choose a reason for hiding this comment

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

LGTM.

@therealsujitk
Copy link
Collaborator

@vatz88 could you take a look at this too before we merge? Thanks!

@vatz88
Copy link
Owner

vatz88 commented Jun 28, 2024

Looks fine to me. Please test it thoroughly on deploy preview before merging

@anantgupta001
Copy link
Contributor Author

@therealsujitk If everything is functioning correctly, could you please merge all changes and close this PR? Should there be any issues or further adjustments needed, please let me know.

@therealsujitk therealsujitk merged commit 3a0a553 into vatz88:master Jun 28, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for VIT AP campus
3 participants