Skip to content

Conversation

@samindiii
Copy link

@samindiii samindiii commented Apr 27, 2025

Description

This PR implements backend functionality to manage tutorials via CSV files. It introduces new POST and GET endpoints for importing and exporting tutorials, respectively.

Key Features:
POST Endpoint (Import Tutorials from CSV):
A new POST endpoint is added to handle the import of tutorial data from CSV files.
The endpoint processes incoming CSV files, parses the data, and creates new tutorial records in the database.
Validation ensures that the CSV data is correctly formatted and matches the tutorial model before saving it to the database.

GET Endpoint (Export Tutorials to CSV):
A new GET endpoint is introduced to export tutorials as CSV files.
The endpoint generates a CSV file containing tutorial data, which can be downloaded by the user.
The exported CSV includes all relevant fields associated with the tutorials.

Integration with Tutorial Model:
The tutorial model is extended to support the import and export functionality.
Methods for parsing CSV data and converting tutorial records into CSV format are implemented directly within the tutorial model.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

In your terminal, change directory to doubtfire-api. If you are in doubtfire-deploy, run the following:

cd doubtfire-api

Run the following to run all existing tests

rails test

Screenshot 2025-04-27 at 2 42 43 PM

Run the following if you want to specifically run my test

rails test test/models/tutorial_model_test.rb

Screenshot 2025-04-27 at 12 03 44 PM

Additionally, if you want to test the integration, follow these steps:

  1. Pull the front-end code from Feature/import download tutorials csv frontend doubtfire-web#314
  2. Login as admin (you could also test that the tutorial panel does not show up when you login as tutor or student). The panel looks like this -
  1. Navigate to the tutorial panel at the top and upload csv file.
  2. I have included a test file inside doubtfire-api/test_files/Tutorials.csv.
  3. Download this file to your computer and attempt to upload using the upload csv button.
  4. Test the download_csv button, it should download all current tutorials.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if appropriate
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have created or extended unit tests to address my new additions
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@DauDAtem
Copy link

Clear Separation of Endpoints
Import and export are logically separated into POST and GET handlers, making the API intuitive.

Built-in Model Methods
Embedding CSV parsing/serialization into the Tutorial model simplifies reuse and keeps controller code concise.

Validation Before Save
Ensuring CSV rows conform to the model prevents malformed data from entering the database.

Test Coverage Instructions
Clear instructions for running both the full suite and the specific tutorial model tests help reviewers verify functionality quickly.

Copy link

@DauDAtem DauDAtem left a comment

Choose a reason for hiding this comment

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

Clear Separation of Endpoints
Import and export are logically separated into POST and GET handlers, making the API intuitive.

Built-in Model Methods
Embedding CSV parsing/serialization into the Tutorial model simplifies reuse and keeps controller code concise.

Validation Before Save
Ensuring CSV rows conform to the model prevents malformed data from entering the database.

Test Coverage Instructions
Clear instructions for running both the full suite and the specific tutorial model tests help reviewers verify functionality quickly.

@amriith
Copy link

amriith commented May 4, 2025

Hi @samindiii great work on your PR — it looks solid overall!
Just a couple of suggestions worth considering:

  1. It would be helpful to implement a reasonable file size check during CSV uploads to prevent potential performance issues.

  2. Consider wrapping the import logic within a transaction. This way, if any row fails during processing, it won't result in a partially imported dataset.

Everything else looks good — nicely done!
image

Copy link

@b0ink b0ink left a comment

Choose a reason for hiding this comment

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

A few nitpicks on single vs double string quotes, and a couple discussion points regarding how we retrieve the tutors (unit roles) and if the endpoint should be an extension to the unit entity instead of retrieving the unit id from the CSV

Comment on lines +1 to +3
code,abbreviation,unit_id,tutor_id,tutorial_stream
test,CS3,4,8,
,FX1,1,2,Practical-1 No newline at end of file
Copy link

Choose a reason for hiding this comment

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

Could we use the name of the tutor instead of their ID to make things easier if any changes to the CSV need to be made before importing?

This would require the tutor to already be added as a unit role to the unit, and throw a warning for each row that has a tutor name that isn't part of the unit

Copy link
Author

Choose a reason for hiding this comment

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

I could do that but isn't it much better to use ID since it is a unique identifier? If two tutors have the same name or if a tutor has two accounts with the same name but different IDs, how would we know which one to choose? I feel like it is bad practice to use name, which is not a primary key to identify a user, correct me if I'm wrong :)

params do
requires :file, type: File, desc: 'CSV upload file.'
end
post '/csv/tutorials/upload' do
Copy link

Choose a reason for hiding this comment

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

Should the endpoint be an extension to the unit entity? So that the unit_id is used from the endpoint and not required in the CSV

It would allow the exact same CSV to be imported into two different units

Copy link
Author

Choose a reason for hiding this comment

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

I don't really understand why this is necessary. Based on my understanding, two different units cannot have the same tutorials. Will there be a situation where multiple units would have the exact same tutorials given that the information provided in the csv is tutorial specific and tutorials are unit specific?

@amriith
Copy link

amriith commented May 14, 2025

Hi @samindiii Good work on the fixes , I’ve tested the changes and everything is working as expected. Approved from my end

@samindiii samindiii force-pushed the feature/import-download-tutorials-csv-backend branch from abcbd7e to 58b5b07 Compare May 15, 2025 12:52
@aNebula
Copy link

aNebula commented Jun 12, 2025

@samindiii LGTM. Please open an upstream pull request against 9.x branch in /doubtfire-lms/doubtfire-api.
Remember to include the description and link your frontend PR (and vice versa).

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.

5 participants