Curriculum Report #8268
Open
Curriculum Report #8268
Conversation
This file contains 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
|
Should I be able to add both a Subject and Course new report form widget at the same time? |
|
Thanks for catching that @michaelchadwick, I'd forgotten to put that in my todo notes. Got it now! |
jrjohnson
force-pushed
the
courses-report
branch
2 times, most recently
from
eed6004 to
5038802
Compare
jrjohnson
force-pushed
the
courses-report
branch
2 times, most recently
from
dc8e6b6 to
434cc20
Compare
jrjohnson
requested review from
michaelchadwick and
stopfstedt
as code owners
|
This got quite out of hand, code wise, the individual commits hopefully provide some context. Unfortunately the UI concept changed half way through so some of the early commits are now for components that didn't make the Final Cut. |
|
@jrjohnson I can't seem to reproduce the bug with selections in the URL despite no selections in the UI, so... |
stopfstedt
requested changes
Feb 26, 2025
| return rhett; | ||
| }); | ||
| const csv = PapaParse.unparse(data); | ||
| this.finishedBuildingReport = true; |
There was a problem hiding this comment.
this component property is not declared, and it appears that its value is not being processed in the template. please remove.
There was a problem hiding this comment.
Was actually a mostly implemented feature, I hooked it up. Thanks!
packages/frontend/app/components/reports/curriculum/session-objectives.js
Show resolved
Hide resolved
| @service session; | ||
|
|
||
| beforeModel(transition) { | ||
| this.session.requireAuthentication(transition, 'login'); |
There was a problem hiding this comment.
double-checking - is the removal of the auth check here intentional?
There was a problem hiding this comment.
Nope, must have gotten re-generated and I missed this in my review. Great catch!
| async model() { | ||
| const schools = await this.store.findAll('school'); | ||
| const threeYearsAgo = DateTime.now().year - 3; | ||
| // Limit query to surounding years |
There was a problem hiding this comment.
pedantic jerk alert - this is a typo.
Suggested change
| // Limit query to surounding years | |
| // Limit query to surrounding years |
There was a problem hiding this comment.
✅, didn't commit yours because I was already rebased locally. Fixed.
Comment on lines
+424
to
+426
| assert.strictEqual(result[0][0].name, 'jayden'); | ||
| assert.strictEqual(result[0][1].name, 'jasper'); | ||
| assert.strictEqual(result[1][0].name, 'jackson'); |
| year: c.year, | ||
| sessionTitle: s.title, | ||
| firstOfferingDate: firstOfferingDate | ||
| ? DateTime.fromISO(firstOfferingDate).toLocaleString(this.intl.primarlyLocale) |
There was a problem hiding this comment.
typo
Suggested change
| ? DateTime.fromISO(firstOfferingDate).toLocaleString(this.intl.primarlyLocale) | |
| ? DateTime.fromISO(firstOfferingDate).toLocaleString(this.intl.primaryLocale) |
| link: `${origin}${path}`, | ||
| instructors: s.instructors, | ||
| firstOfferingDate: firstOfferingDate | ||
| ? DateTime.fromISO(firstOfferingDate).toLocaleString(this.intl.primarlyLocale) |
There was a problem hiding this comment.
typo
Suggested change
| ? DateTime.fromISO(firstOfferingDate).toLocaleString(this.intl.primarlyLocale) | |
| ? DateTime.fromISO(firstOfferingDate).toLocaleString(this.intl.primaryLocale) |
Switched from expand collapse to the same type of dropdown picker we have in other places in the app. User chooses what type of report they'd like to add and then adds it. Courses report super blank right now.
Pick courses to report on and display them.
Pull all the data from the GraphQL API for all of these selected courses and put a summary on the screen. There are some guesses here. I've selected the first offering of ILM to determine start date, for example. Will have to test the output and see if it's correct.
This component was getting pretty huge, let's split the results out into another container.
I wasn't understanding this report. Instead each session objective gets a line, that's the focus of the output.
Adding data for instructors, first offering, and duration to the report and re-doing the query to be course based to fetch all this additional information efficiently.
Instead of searching for a course all of the schools are listed and you pick each course you want to report on from that list.
Moving the subject reports and the list into a tree so we can differentiate the curriculum reports into their own space. This is first step cleanup to get it working, lost more to add to make it good.
This is the new container, just moving everyone over for now and getting rid of some old stuff.
It's not a lot of data and takes less than a few seconds to load. By having it all upfront we avoid loading spinners in the course chooser and screen jumps in the list of selected courses.
Moving buttons around and putting the results OR the course picker on the screen.
Moving the action buttons out of their containers so they appear into the same space when a report is run, even though they're in different levels of the component tree.
Sort the primary school to the top of the list and fixed the HTML structure to be a correct list and not an invalid bunch of tags.
This isn't part of the language, have to add it ourselves.
Trying out a progress bar loading approach and chunking requests into smaller pieces. Loading the entire year's worth of courses breaks in some unspecified way without breaking it apart, plus we get to know how we're doing loading.
Without this you can get stuck in a very long load without a clear way to escape.
Also realized we should be choosing the current academic year with our utility function, which also makes testing this easier.
I didn't love the ergonomics of this and it only gets worst the more tests you add. I've replaced it with a utility method that builds the data out of what is in the mirage server so data loading looks like it does in other tests.
Without this it looks like a course didn't load.
We're going to need this in other curriculum report tests.
Mostly tests, but found an oddity in the API where we were exposing the selection internals up a layer (probably copy / paste). Cleaned that up.
This belongs under curriculum reports
I'm still not thrilled at all this manual fetch code, but at least it's all in one place now.
We'd need the router mocked and all we'd be testing is that the UI renders.
UL elements can't container other UL elements. They have to be inside the list item. Added the a11y check and this popped up. Fixed.
Needed a label on the select and a visible button control.
Adding some basic component tests
Chunking by zero is never going to happen, but it was running an infinite for loop and we already had the test so I added a fix.
This is just some links in a box, but we need the page object for other tests so I added it.
jrjohnson
force-pushed
the
courses-report
branch
from
6815c52 to
93afb0d
Compare
✅ Deploy Preview for ilios-frontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jrjohnson
force-pushed
the
courses-report
branch
from
93afb0d to
8236555
Compare
Got rid of the report chooser cruft and pointed the test at the correct page object to run.
These are pretty basic, but add some percy snapshots and a bit of data checking.
When we run a report download the download icon is briefly replaced with a check mark, but that flag wasn't being tracked nor passed to the header.
Not sure when this got removed, but it's needed.
jrjohnson
force-pushed
the
courses-report
branch
from
8236555 to
4b43498
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.
Creating a new type of Curriculum report that covers multiple courses. Each report is a separate component so it can control its own data loading, summary, and download.
WIP: