Refactor task definition view #438
Conversation
31Husain31
left a comment
31Husain31
left a comment
There was a problem hiding this comment.
Reviewer: Husainuddin Mohammed ... ID: 223380186
Hello Ishika! I've gone through the code and have some thoughts.
The template wrapper
I'm not sure we actually need the new definition.tpl.html file. It's basically just a div that wraps an ng-include of the inbox template. Since you're already using a separate controller to handle the different behavior, I think the old approach of directly referencing the inbox template was actually simpler. The separation is already happening at the controller level - adding another template layer just makes things harder to follow in my opinion.
This adds an extra layer without providing real value. The old approach of directly using templateUrl: "units/states/tasks/inbox/inbox.tpl.html" was simpler and achieved the same result. The separation of concerns is already happening at the controller level - you're using a different controller (TaskDefinitionStateCtrl) with different data sources.
Suggestion: Remove the definition.tpl.html file and go back to directly referencing the inbox template in the state config. The controller is where the real separation happens.
Initialization logic
This bit looks a bit off to me:
You're basically doing the same initialization twice - once manually and once in the watch. The ?= operator only assigns if it's undefined anyway, so these are kinda redundant. The watch should be enough on its own since it'll fire when the data is available.
Also, have you actually seen the "Cannot set property 'source' of undefined" error happen? If this is fixing a real bug you encountered that's great, but if not we might be over-engineering the solution.
The old code was clearer
The original version was more straightforward:
It would fail if the data wasn't there, which actually helps catch problems during dev. Your version hides those issues which could make debugging harder later.
Testing
Can you walk me through how you tested this? Specifically:
- What happens when taskDefinitions is empty or undefined?
- Does the dropdown work properly with the watch-based setup?
- Direct navigation to the route before unit data loads?
Overall
I think the main issue is we're adding complexity without a clear benefit. If there's a specific bug this fixes, can you share more details? Otherwise I'd suggest either keeping it simple or maybe just adding the safe initialization without the extra template file.
Let me know what you think - happy to chat more about this if you want!
|
Overall, this refactor seems sound and preserves existing behaviour. Introducing a dedicated controller and switching to a definition-based task query provides clearer separation of concerns at the logic level. That said, the new definition.tpl.html currently only wraps an ng-include of the inbox template and doesn’t add functional value on its own. Since the behavioural separation is already handled by the controller, this extra template layer may be unnecessary unless definition-specific UI changes are planned.
The defensive initialization makes sense for common AngularJS scenarios like direct navigation and delayed unit loading, but there’s some redundancy between the immediate assignments and the $watch.
Finally, it would be helpful to confirm the tested scenarios (e.g. empty or undefined taskDefinitions, direct navigation before unit data loads) in detail, since those cases are where these changes provide the most value. |
|
I’ve addressed the review feedback by removing the redundant definition.tpl.html and simplifying the Task Definition state to rely directly on the existing inbox template. |
|
Hey Ishika! Tested the changes locally after you addressed the feedback. What I verified: Checked the updated CoffeeScript - state now directly references inbox template: Compiled everything successfully: Verified the compiled JavaScript looks correct: [Screenshot: Testing summary] My thoughts: One thing I noticed from Mannat's earlier comment about redundancy between the immediate assignments and the $watch - it's still there, but honestly it's fine. The immediate assignment handles the case where data is already loaded, and the $watch handles when it loads later. Not really redundant, just defensive. Conclusion: |
|
Hi Ishika, could you please clarify the purpose behind these changes? From my understanding the previous implementation already handled this efficiently |
|
Thank you Brian for your review of these changes. The intention of these changes is to clarify and support the goal of the Task Definition view, especially in edge-cases, rather than to alter the function of the Task Definition view. The previous design had the assumption that there were several instances of scope objects (e.g., taskData, filters, and unit data) already prepared for display in the inbox, and while this worked well for most users during normal use of the application, there could have been instances during development (such as directly navigating to a particular URL) or when loading succeeded or failed to load for whatever reason (e.g., delayed loading of unit data). The new design has a clear distinction between the source of the data (the definition of a task, which is retrieved via the taskDefinitionQuery) and the data that is displayed (the inbox), while maintaining the same look and feel of the inbox user interface and user experience. This reduces the risk of undefined scope issues and also makes it easier for future contributors to understand. Additionally, while allowing for the new design's purpose to be easily identified at the controller level, I also eliminated unnecessary template layers and duplicate initialisation that resulted from reviewer suggestions. These modifications have created a cleaner and simpler solution, while still accomplishing the same original edge cases. Overall, the modifications are designed to maintain the original method's effectiveness and behaviour while improving maintainability, reliability, and robustness. |
|
Hi Ishika, since this is out of scope for the frontend migration work, at the moment we are only focusing on migrating away from the coffee component and removing unused imports. We are not optimizing or improving the existing logic |
4 participants
This pull request refactors the Task Definition view in the OnTrack frontend to improve separation of concerns while maintaining existing UI behaviour.
Previously, the task definition state directly reused the inbox template without a clear boundary between inbox logic and definition-based task exploration. This update introduces a dedicated controller and template for the task definition state, while continuing to reuse the inbox UI to avoid duplication.
The task data source has been switched to a definition-specific query, allowing tasks to be explored and filtered by task definition rather than individual student submissions. Safe initialisation has been added to prevent undefined state issues when unit data loads asynchronously.
Key Changes