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

Replace New Subject Selectors #7324

Merged
merged 14 commits into from
Aug 30, 2023
Merged

Conversation

jrjohnson
Copy link
Member

@jrjohnson jrjohnson commented Jul 31, 2023

Split these out into their own component so each one can be customized.

This is step one to resolve #4358, but maintains the existing behavior.

Wip:

  • Fixup acceptance tests
  • Test to ensure current behavior is maintained (or improved)

@jrjohnson
Copy link
Member Author

@dartajax this still needs code review, but it is ready for some testing. To test, pick various "Associated With" in a new subject report. Each of the options should look the same (or better) than it does in production today. If there are any regressions we'll want to fix those before moving on to the next step which is actually fixing the ones that don't work now.

@dartajax
Copy link
Member

dartajax commented Aug 8, 2023

Comment log (so far) ...

  • Sessions by Terms reports work here but don't in Prod = improvement
  • Selected instructor badges are a bit smaller = improvement
  • Course associated with Learning Material - on prod no search field ever appears - on this PR, search field appears but there is nothing returned to search against = slight improvement
  • Course associated with Learning Material - on prod screen froze completely - on the PR, no problem going to next test = improvement
  • Course associated with Competency - competency sorted A-Z using PR - Z-A on prod = improvement
  • Course associated with MeSH Terms - on prod works - on PR search is not completed = needs review or fix
  • Courses by Terms reports work here but don't in Prod = improvement
  • All Sessions works in both = no change = approved
  • Added ticket for indicator of data load - Subject Reports - We Need an Indicator for Work In Progress ilios#4908
  • Sessions in School associated with Course in [Year] which is [choose] - needs to be alphabetized - drop-down not sorted A-Z and should be = needs fix
  • side note on the previous comment - the [Year] value has been removed from the selector (drop-down) but the year has already been selected so this actually may be better - team discussion? = probable improvement
  • Sessions in School associated with [Program] which is [choose] defaults to Doctor of Medicine in Netlify latest build even though that is the second program in A-Z order - in this PR it starts with Curriculum Ambassadors Program which is the first program in A-Z order so I guess that would be an improvement = just a weird thing I noticed
  • Sessions by [Instructor] works fine with smaller badge = improvement
  • Sessions by [Instructor Group] works fine - better sorting no middle default = improvement
  • Sessions by [Learning Material] - at least there's a spinner while on prod and netlify no spinner or entry field once completed - I guess a spinner that never completes is better than nothing - Subject Reports Netlify Rage Click Error ilios#4909 - I couldn't get this report to run in any environment
  • Sessions by [Competency] = improvement - better sorting
  • Sessions by [MeSH] = seems fine
  • Sessions by [Session Type] - seems fine - better sorting = improvement
  • Sessions by [Term] - works in this PR but not otherwise = improvement
  • All Programs - All Schools = works fine
  • All Programs [Choose School] = also fine - added this If All Schools Report Is Run, Provide a Drop-down to Select School ilios#4912
  • All Programs associated with Session = drop-downs took a long time to load but they did load - needs indicator - got a browser error too asking to stop loading the page = page is not allowing any drop-downs to be utilized - well it's a bit better than demo or netlify but "This page is slowing down Firefox. To speed up your browser, stop this page" - Stop button provided so = needs review, repair, or removal
  • Programs by School associated with Term = improvement - broken elsewhere

This allows us to control the UI and data loading individually.
This is an interim step, it needs to be replaced with search.
and finish adding program as well
Forgot to test that these actually do something, they do!
Taking advantage of new page objects, and cleaning up error checks for
new structure.
I've sorted by year and title by fixing the passed values, however since
the courses display with an external ID this isn't always satisfactory
visually. I can't come up with any combination of values that I like
more though, so sticking with this for now.
@jrjohnson
Copy link
Member Author

  • Course associated with MeSH Terms - on prod works - on PR search is not completed - working now, may have been a problem with search on demo
  • Sessions in School associated with Course in [Year] which is [choose] - needs to be alphabetized - drop-down not sorted A-Z and should be
  • All Programs associated with Session - known issue, will fix with better session selector

@jrjohnson jrjohnson marked this pull request as ready for review August 10, 2023 07:03
@dartajax
Copy link
Member

dartajax commented Aug 10, 2023

Comment log (continuing) ...

  • Program Years - All Schools or [School] - looks good
  • Program Years by School All [Program Years] associated with [Course] in [Year] which is [Course Title filtered by the other criteria] - there was a default in prod (actually I think the sorting was different) - it's better now - strange report parameters and data preparation dashboard (or whatever you want to call it) - all in all = improvement
  • The report mentioned above seems to be a really long-winded way of asking "What program year is associated with this Course?"
  • Program Years >> [choose School] >> [choose Program Years (why? that's what we're looking for) or select All (thankfully the default) >> Associated with [Session] (choose Session) >> in [All Academic Years] or select All (again thankfully the default but for the second time why?) >> which is [Session] - this one actually runs providing a really long list (if you accept the defaults) of sessions from which to choose to find out what ends up being not the program year or academic year of the course but the Class of [Year] value. Anyway phew = improvement
  • The report mentioned above must be the world's longest way of trying to figure when the students in any given course would be scheduled to graduate.
  • Program Years >> {School] >> associated with Term which is ... [Term] - this one works in the PR but not in demo/prod so = improvement
  • Instructors >> [School or All Schools] >> Associated with [Anything or choose something] - for my first test, I went with Anything thinking this might be a convenient way to get a listing of all instructors in my (or all) schools. When you try this, there is a message stating "Association is required when Instructor is the subject" - I will file this as a bug. Why include something in a drop-down that raises an error message by default when you try and save it?
  • Subject Reports - Instructor - Trying to Save a Report Using Defaults Raises Error ilios#4913
  • Instructors >> [School] >> [Year] >> [Course] - taking a long time to retrieve anything - sort order better in PR (A-Z) - other than that not really returning data in a reasonable amount of time (unless you have 15 minutes to spare) - neither returned data in 30 minutes - shouldn't take so long to retrieve Instructors for a Course, no? Anyway, I'm moving on = neither better or worse - both too slow

Copy link
Member

@stopfstedt stopfstedt left a comment

Choose a reason for hiding this comment

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

LGTM. breaking the monster component down into smaller, more digestible parts is a big improvement.

@dartajax
Copy link
Member

dartajax commented Aug 14, 2023

Comment log (continuing)

  • Instructors (by School) for Session - the "Academic Years" label and drop-down appear immediately in other instances but you can't select anything because it's not loaded up yet - in this PR it loads up later and this is fine - it is quite slow though once you choose the Academic Year - again it seems to me at least strange since all the sessions appear loaded, can't they just be filtered at this point? None of them really seem to work on this report - PR, netlify, demo, prod - it's not worse though that's for sure = improvement
  • the above - why so slow - instructors for a session
  • Instructors in Instructor Group - filtered by school - no problem anywhere so = approved
  • Instructors (filtered by School) - associated with Learning Material - doesn't really work anywhere - empty drop-down for "which is ..." eventually loads up in the PR version. Nothing to select or search though. This may be helpful ... or not ...
    image
  • Instructors (filtered by School) - Session Type - works fine - better default selection (none) = improvement
  • Instructor Groups (filter by school or not) associated with Course - Academic Year and Course list load up super fast here - can we re-use this faster logic for some of the slower to load report designers?
  • Instructor Groups (filter by school) associated with Session - drop-down for Academic Year available earlier on netlfiy - actually this one works on netlify but not on this PR = needs to be checked and tested
  • Instructor Groups associated with Instructor - works fine = approved
  • Instructor Groups associated with Learning Material - retrieves an empty drop-down for selection process - there is no way a drop-down menu could ever contain all Learning Materials to select from anyway - doesn't work on netlify either = needs review / fix
  • Instructor Groups associated with Session Type [Selected from drop-down] = no problem - approved

@dartajax
Copy link
Member

dartajax commented Aug 14, 2023

Comment log (continued testing) ...

  • Learning Materials by Course - works fine = approved - assuming this report is for course level learning materials only
  • School >> Learning Materials >> Session - drop-down shows up initially on netlify not on PR - neither is ready for action - waiting it out - it eventually loaded up in the PR - not in netlify - still we need an indicator if reports are in process but that has been ticketed by me Subject Reports - We Need an Indicator for Work In Progress ilios#4908 - the session drop-down is still not activated but appears to have been filtered by year - you can press save and save the report based on the first value (A-Z) but this is not very helpful - it's probably still loading or processing - it finally came around so = approved - this worked in netlify as well eventually - all good on the PR in any case as well just slow with no indicator
  • Learning Materials for Instructor seems fine = approved
  • Learning Materials for Instructor Group seems fine = approved
  • Learning Materials based on MeSH Term = search for MeSH not completing in timely manner - also true on netlify so maybe something else causing the search to not return values - either way = needs review / possible fix / test again later
  • the above report ONLY worked on prod - maybe search is broken on demo currently? - test later
  • probably not super helpful since demo search is in flux right now anyway ...
    image
  • All Learning Materials (in School) by Session Type [type] seems fine = approved
  • Goofy blue dot floating around in tab - not present in netlify, prod, or demo ...
    image
  • Competencies [school] >> [academic year] >> [course] - runs in both the PR and netlify pretty slowly = approved
  • might be helpful (or not) ...
    The resource at “https://deploy-preview-7324--ilios-frontend.netlify.app/assets/fonts/nunito-sans/nunito-sans-latin-700.woff2” preloaded with link preload was not used within a few seconds. Make sure all attributes of the preload tag are set correctly.
  • Not getting too much help using the inspector or don't know how to properly report what is going on - either way I'm sitting and spinning on top of the world ...
    image
  • This doesn't seem to run at all on netlify so ... = improvement but needs speeding up - I don't want to me that individual but don't we have enough parameters or already loaded data to retrieve these values in a speedier manner?
  • XHR seemed to be hogging up the resources - not sure why inspector wasn't staying with us regarding how long it was taking - wasn't refreshing
  • Competecies >> [school] >> [session_type] - no prob = approved
  • MeSH Terms by Course >> [school] >> [academic year] - data not getting returned in a timely manner anywhere - screen shot from PR console ...
    image
  • MeSH Terms by Session .. [school] >> [academic_year] >> [session] - guaranteed to be similar but even slower than the one above - the design set up was fine

@dartajax
Copy link
Member

Final testing journal ...

  • MeSH terms >> [school] >> [learning_material] ... setting this report up would take too long - not gonna stop me from approving the PR though - just saying = "approved"
  • [school] >> Terms by Course >> in [academic_year} >> [course_title] - works great = approved
  • compare these two screen shots if you dare ...

from this PR ...
image

from demo site ...
image

  • [school] >> Terms by Session >> in ...[academic_year] then [session_title] - better probably to choose [course] and then session - we can look at this down the road - for now trying to finish setting up the report - eventually - this PR is a bit faster than demo - I have managed to select the academic year but drop-down refresh is really slow - on demo not even this far yet - getting page is slowing down Firefox messages on both -- for reference and entertainment ...

image

I closed the warning message and it did eventually complete so = improvement

  • [school] >> Terms by Program Year - totally busted in prod - in PR ... see below = needs review / fix = bug / issue with PR

image

image

@dartajax
Copy link
Member

more report testing ...

  • all Terms by Program - works fine = approved
  • all Terms by Instructor - works fine = approved
  • all Terms by Learning Material - empty drop-down provided - not really working - should be search box probably = fix later / revamp but after PR merge
  • all Terms by Competency - works fine = approved
  • all Terms by MeSH term - search / report setup is working - data retrieval not so fast - gonna wait it out = beats me / merge anyway
  • all Terms by Session Type - works fine = approved
  • all Session Types by Course - works fine = approved
  • all Session Types by Program - works fine = approved
  • all Session Types by Instructor - works fine = approved
  • all Session Types by Instructor Group - works fine = approved
  • all Session Types by (for?) Learning Material - dream on - apples and oranges - most learning material reports well and MeSH reports phew you better be patient - is this report even necessary - I guess the session types that have been used with a given LM might be valuable information - empty drop-down on PR - never ending search in prod = merge anyway / let's get this going
  • all Session Types by Competency - works fine = approved
  • all Session Types by MeSH - works fine = approved
  • all Session Types by Term - works fine = approved

Copy link
Member

@dartajax dartajax left a comment

Choose a reason for hiding this comment

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

After exhaustive / exhausting testing, there is one remaining straggler - terms (in school) by program year - it's busted in prod currently - this will be ticketed once verified to still be an issue post-merge - other than that, lots of improvements are included here which will lead to tightening up our reports going forward - gonna merge

@dartajax dartajax merged commit 6a90392 into ilios:master Aug 30, 2023
14 checks passed
@jrjohnson jrjohnson deleted the 4358-subject-search branch August 30, 2023 23:59
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.

3 participants