SAK-48907 Lessons restore lessons subpage navigation #11682
Conversation
hornersa
force-pushed
the
SAK-48907
branch
4 times, most recently
from
9113dfb
to
95a5e63
Compare
|
I've narrowed down a few dozen Codacy errors down to five, about which I don't know how to proceed-- one of which (the medium one) looks like a false error, and it's based on code that existed prior to SAK-48804. |
There was a problem hiding this comment.
Ideally, I'd prefer the generation of the lessons subnav to happen on the server, like the rest of the portal navigation. I really don't like the idea of this lump of json being passed to the front end and then more rendering happening there. I know this is just how it was before, but it's still nasty. Also, the lessons specific selectors which just assume that lessons will keep on using same url patterms, they should should really come from lessons. Lessons should supply those selectors in some kind of api, so the portal/library code just asks for them. Generally, and I know I am guilty as anyone here, we should be supplying urls for data, or whatever, from the backend to the UI. The UI should be ignorant of what url it needs to get stuff. That way we can refactor a lot more easily.
I realise that's a lot. As I said in the JIRA, I was just hoping, maybe naively, that this would just be able to go away and we'd just use the lessons tool itself to navigate the subpages, thus simplifying the portal nav code, which is already pretty complex. But, if it must stay, it must, but I think it needs some refactoring so it is produced on the server, and not on the client.
Again, I know this isn't you @hornersa, it's just how it was, so it's no criticism at all. I'd just like it to be better than it was, less brittle.
| @@ -50,12 +50,14 @@ public class LessonsSubNavBuilder { | |||
| private String siteId; | |||
| private boolean isInstructor; | |||
| private Map<String, ArrayList<Map<String, String>>> subnavData; | |||
| private ArrayList<Map<String, String>> topLevelPageProps; | |||
There was a problem hiding this comment.
| private ArrayList<Map<String, String>> topLevelPageProps; | |
| private Collection<Map<String, String>> topLevelPageProps; |
There was a problem hiding this comment.
I chose ArrayList instead of Collection because sequence matters with respect to determining which pages are prerequisites of which. The type for topLevelPageProps mirrors the nested ArrayList type for subnavData, where the sequence of the data also matters.
| public static boolean addToEditToolsContext(Context context, Site site, SessionState state) | ||
| { | ||
| if (context == null || site == null || state == null) | ||
| { |
There was a problem hiding this comment.
Indentation is a bit odd here.
| public static boolean addToSiteInfoContext(Context context, Site site, SessionState state) | ||
| { | ||
| if (context == null || site == null || state == null) | ||
| { |
| // Allow checkbox to set the isLessonsSubNavEnabledForSite site property | ||
| // If checked, add this to the state | ||
| if ("on".equals(params.getString(FORM_INPUT_ID))) | ||
| { |
| { | ||
|
|
||
| if (site == null || context == null || state == null) | ||
| { |
| public static boolean removeFromState(SessionState state) | ||
| { | ||
| if (state != null) | ||
| { |
|
|
||
| // If a sibling page with a smaller sequence is required | ||
| // we want to disable the current page for students | ||
| if (pageData.get("prerequisite").equals("true") && prerequisiteApplies) { |
There was a problem hiding this comment.
Will prerequisite always be a key? What if the page is hidden?
There was a problem hiding this comment.
The keys “prerequisite”, “required”, “hidden”, etc. are set in the sibling method processTopLevelPageProperties, always called upstream by SimplePageToolDaoImpl.getLesson before that method calls LessonsSubNavBuilder.getLessonSubPageJSON, which in turn is the only method calling applyPrerequisites.
I’m not sure if I glean the use case you may have in mind with your question “What if the page is hidden”? To my knowledge, the hidden attribute is independent of the entire logic of the required/prerequisite dependency chain.
The logic within the new lines I introduced to “applyPrerequisites” is a direct copy of the code above for the subpages. Thus, in hindsight, I should have refactored that logic into a separate method. I’ve implemented this refactoring in my revised commit.
|
|
||
| // Only disable pages that have prerequisites below the current page | ||
| // when the current page is required and the user is yet to complete it | ||
| if (pageData.get("required").equals("true")) { |
There was a problem hiding this comment.
Can we assume required will always be a key? If not we should use StringUtils.equals here.
There was a problem hiding this comment.
Yes, for the same reason above.
| private String getLessonsSubpages(String userId, Boolean updatePermisson, String siteId, List<SitePage> pageList) { | ||
| //Lessons is expecting a Map containing the "pageId" and "wellKnownToolId" | ||
| List<Map<String, Object>> pageMapList = pageList.stream().map(page -> { | ||
| Map<String, Object> pageMap = new HashMap(); |
There was a problem hiding this comment.
| Map<String, Object> pageMap = new HashMap(); | |
| Map<String, Object> pageMap = new HashMap<>(); |
There was a problem hiding this comment.
To be resolved in my new commit.
|
|
||
| if ((lessonsSubnavToolIdInput !== null) && (lessonsSubnavPageIdInput !== null) && | ||
| (lessonsSubnavItemIdInput !== null)) { | ||
| subpageElement = document.querySelector('#toolMenu a[href*="/tool/' + lessonsSubnavToolIdInput.value + |
There was a problem hiding this comment.
This worries me. This selector just seems so coupled to Lessons, for something that is part of the portal nav. If somebody decided to mess with those lessons specific urls in future, would they remember to come back to the portal code and update this selector?
There was a problem hiding this comment.
> This worries me. This selector just seems so coupled to Lessons, for something that is part of the portal nav.
Understood, though the dependency at root here is not new, as I think you’re aware. It’s just adapted from morpheus to the trinity context._
The three hidden fields that the highlighted condition is testing for is to assess whether or not we’re on a Lessons page rendered by the ShowPage template. As-is, that’s the only template from which we can glean what the current Lessons page is for us (in the portal) to highlight in the sitenav. In other words there is no continuity for displaying those hidden fields in the DOM across the Lessons tool for the other 20+ Lessons templates (e.g., ShowItem, Reorder, IndexOfPages, etc.), so the client side portal code resorts to highlighting the “Main Page” based on the
> If somebody decided to mess with those lessons specific urls in future, would they remember to come back to the portal code and update this selector?
If somebody did change the URLs, our institution would certainly notice-- per the conversation in the jira. :) But yes, point taken.
|
Responses regarding the wider context of this PR are in the conversation in the corresponding jira. The defaults of my emacs editor (and my copying-pasting) were responsible for the odd indentation issues raised by the first commit of this PR. My revision to this PR will restore the prior version of this file, plus one fix to make the brace positions consistent within the class. Other issues I’ll respond to where they’re raised. |
ern
changed the title
|
What's the state of this PR? Thanks |
|
I've been maintaining my own local branch of this feature, using the same paradigm as was supported in the past: the Lessons tool server side provides json to the portal, and the portal's client side code parses and renders the subpage navigation. |
Besides restoring the Lessons subpage nav code removed from SAK-48804, this proposed PR repairs UX issues that may not have yet been addressed when adapting the Lessons subpage nav to the new portal, Trinity.
For a demo of the proposed fix, refer to the screencast, SAK-48907-ProposedFix.mp4, attached to the corresponding jira.