Implement API Endpoints for bulk-import of Schools #622
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
2 times, most recently
from
3de1b35 to
68a1e78
Compare
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
2 times, most recently
from
2f964a2 to
b613fb8
Compare
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
from
b613fb8 to
c9e400a
Compare
raspberrypiherokubot
temporarily deployed
to
editor-api-p-fs-impleme-nqxegq
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 11
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
from
c9e400a to
1cbaf58
Compare
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
2 times, most recently
from
d0a84c3 to
90e1a35
Compare
app/jobs/school_import_job.rb
Outdated
| school.verify! | ||
|
|
||
| # Create owner role | ||
| Role.owner.create!(school_id: school.id, user_id: owner[:id]) |
There was a problem hiding this comment.
Bug: Owner role creation fails for cross-school users
Creating an owner role will fail if the user already has any role in a different school due to the users_can_only_have_roles_in_one_school validation in the Role model. The import job doesn't check for existing roles in other schools before attempting to create the owner role, causing legitimate imports to fail when users are teachers or students elsewhere.
Contributor
Author
There was a problem hiding this comment.
This is supposed to be a failure condition, but all of this is wrapped in a transaction, so it's atomic.
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
from
90e1a35 to
48f5d00
Compare
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
2 times, most recently
from
4096e1d to
3681a33
Compare
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
2 times, most recently
from
62bb9ea to
166509b
Compare
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
2 times, most recently
from
36f73e2 to
3604789
Compare
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
from
f3f67bc to
848ab06
Compare
raspberrypiherokubot
temporarily deployed
to
editor-api-p-fs-impleme-whr8un
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
from
848ab06 to
c05a9d1
Compare
Contributor
Author
|
@danhalson Thanks for the comprehensive and careful review. I have left your comments unresolved but added links to the commits where I addressed your points. Hopefully that makes it easy to track in re-review? |
We allow `editor-admin` and `experience-cs-admin` users to import schools and monitor the state of the resulting jobs.
This change adds a search_by_email method to allow lookup of users by email address to acquire the ID of proposed school owners.
This commit introduces a SchoolImportJob type of GoodJob job to allow for enqueueing imports and tracking their asynchronous completion. The controller will return to the caller a Job ID which can be used to track the progress of these jobs and retrieve their results when done. The SchoolImportResult class tracks the job and provides access to the results. There are structured error codes for possible issues in SchoolImportError.
This operation parses and validates the CSV and reports on errors or enqueues the job.
This commit modifies SchoolsController to add the endpoint for importing schools. It introduces SchoolImportJobsController as the controller for the endpoint though which clients can track job status.
These tests cover the core functionality and some additional complex edge cases.
This commit adds new UI in /admin to allow users to upload CSVs of schools and track the results of the upload jobs in the UI. They can also download CSV files of the resulting school structures. Add index, show and new views for Administrate Add SchoolImportResultDashboard Add routes for admin interface for school_import_results Add SchoolImportResultsController to show results in the UI. This commit also adds code to new.html.erb to show validation errors if the uploaded CSV can't be enqueued as a batch.
Only one Job is created per job, but an upload which is failed or retried could result in more than one execution.
…stead of duplicating.
fspeirs
force-pushed
the
fs-implement-school-import-endpoint
branch
from
c05a9d1 to
5f26d02
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
Points for consideration:
editor-adminorexperience-cs-adminroles. All other roles and none should return401.userinfoto look up school owners by email - there is no bulk option for this.What's changed?
/api/schools/import- for POSTing CSV/api/school_import_jobs- for tracking job statuseditor-adminorexperience-cs-adminroles.ImportSchoolsJobGoodJob job type that will execute the import asynchronouslySchoolImportResultrecord type that tracks the results ofImportSchoolsJobs.Steps to perform after deploying to production
There is a database migration involved in this to add the table that tracks
SchoolImportResult.Tasks Outstanding
import_schoolsin some places andschool_importin others).editor-apiOAuth token/adminpathFollow-On Work
This is not the final form of this feature. In a follow-up task in the next sprint we intend to add a UI under the/adminroute to make it easier for non-developers to access this endpoint. The goal in the first instance is to develop and test the endpoints separately from the UI that accesses them.This PR now contains all the work necessary to make a complete feature, including web-based UI.
Note
Adds CSV-based bulk school import with admin UI, async GoodJob processing, job status API, and persisted results (with CSV export), restricted to editor/experience CS admins.
POST /api/schools/importto start imports;GET /api/school_import_jobs/:idto track status.School::ImportBatchwith structuredSchoolImportErrorcodes.SchoolImportJob(queue:import_schools_job) creates/verifies schools, assigns owner role, and saves results toSchoolImportResult.config/initializers/good_job.rb.admin/school_import_resultspages (index/show/new) for upload, status, results, and CSV download.StatusField,ResultsSummaryField,UserInfoFieldwith batched user lookups.SchoolImportResultmodel/table (+ migration) and dashboard.School#importand:school_import_jobread to editor/experience CS admins.UserInfoApiClientaddsfind_user_by_emailand improved transforms.docs/import/.Written by Cursor Bugbot for commit 6eacf9a. This will update automatically on new commits. Configure here.