diff --git a/students/weiquu/knowledge.md b/students/weiquu/knowledge.md index f43945fbd..34d959c19 100644 --- a/students/weiquu/knowledge.md +++ b/students/weiquu/knowledge.md @@ -1,13 +1,41 @@ ### CS3282 -Point form for now, will expand later: - -- More data migration: migrating actions and search functionality -- Tradeoffs made in database design and their consequences -- Testing capabilities: E2E tests, integration tests, unit tests -- Security vulnerabilities: how Hibernate protects against them, testing for SQL injection -- Tech design: process of coming up with user requirements and thinking from user perspective, how to design backend with minimal changes and yet still have it be extensible -- OSS skills: managing a team of people, individually and as a small group +#### Database Migration + +Before CS3282, I had little knowledge of database migration, as I was mainly working on frontend user friendliness issues. Hence, I had to learn the proper way to migrate actions so to as to effectively review PRs and mentor the CS3281 students. This gave me the chance to learn more about the Hibernate ORM, as well as the different components in the backend. For example, one of the PRs that I took on was about migrating search functionality. This involved me learning more about Solr, how it is setup and returns search results in the database, as well as how documents and their indexing are handled. The search functionality turned out to be quite issue-prone, and we had to troubleshoot many of the problems that inevitably cropped up. + +Another point is on database design and their consequences. Early on in the semester, I realised (along with Dominic (Lim)) that a lot of changes were being manually cascaded throughout the database tables. This didn't really make sense to us, as it seemed error-prone and went against what we knew about relational database design. In trying to find out why this decision was made, it seemed that it was because they feared that joining too many tables together might cause performance issues. However, after doing some research, we concluded that performance would not take too much of a hit, and anyway, the performance hit was worth it considering that we are able to better ensure correctness by using the built-in cascade function of the database. Due to documentation being hard to find, it is unclear if there were any other reasons behind manually cascading. Hence, there are a few lessons to be learnt here: + +- Documentation must be more complete and should properly consider all trade-offs. It also must be made more accessible to future batches +- We should follow the theoretically correct way of designing our database - TEAMMATES is not using databases in such a wildly unique way that would warrant doing things otherwise (in fact, I would say our use case is fairly standard) +- More research should be done into performance, and perhaps tests made - our own research showed that there shouldn't be any performance issue even if we use the built-in cascade + +However, as a result of this decision being made already, there is a lot of work to be done to properly refactor the entities. This just goes to show how important prior planning is. + +#### Testing Capabilities + +I learnt much more about E2E tests, integration tests, and unit tests. I also learnt more about the best practices for going about them to ensure standardisation and minimal code duplication (shoutout to Cedric, whose PRs were instrumental in me gaining this knowledge). In addition, I am now better able to pinpoint the issues with our previous tests (seeming to join multiple test types together, as well as not adequately testing all possible pathways, including edge cases), which will be useful in improving testing in the future + +#### Security Vulnerabilities + +As part of the database migration, I had to research into how security vulnerabilities are protected against and how tests are written to ensure that there are no vulnerabilities. In conducting this research, I also looked into how Hibernate and other ORMs protect against security vulnerabilities, for instance by using prepared statements. The other part of my research looked at OWASP guidelines, indicating how different attacks are carried out and how they should be protected against. We decided to focus on testing for SQL injection, and thus I was able to gain more insight into how SQL injections are carried out and how to effectively test for a wide range of cases against them. + +#### Tech Design + +In carrying out the preliminary research into the Multiple Course Structures (MCS) project, I learnt more about how to carry out tech design. The process of coming up with user requirements and thinking from the user perspective is still quite new to me, but I was able to learn a lot from listening to how Prof Damith looked through and considered various aspects of our UI design. In addition, I was able to learn about what features should be prioritised first, and what should be just added later after the MVP is completed. + +On the backend side, I gained valuable experience of how to design backend logic with minimal changes, and yet still have it be extensible. A significant part of this is related to database design, where we should aim to be backward compatible. + +As part of introducing the database schema changes into a separate branch, I was also exposed to the need to introduce database changes step by step, which is even more important at this stage since data migration is happening. For example, new columns should either be nullable at the start or have a suitable default value, and columns and tables shouldn't be dropped until all references in the code are removed. In addition, adding and removing constraints should only be done if the system logic can work before and after such addition or removal. (Thanks Wilson for coaching us through this!) + +#### OSS Skills + +Lastly, I learnt a lot about OSS skills in leading a team of people, both individually (as in the security group and the MCS project) and as a small group (as in the entire database migration). I found that delegating work is not as easy as just deciding who does what - it is also essential to consider what others have done before, what their skills are, what they want to do, as well as whether the tasks will have any blockers. This requires both knowledge of the team members as well as a deep understanding of the system and the changes to be made to it. + +The soft skill of "pushing" people to get work done is also essential, and I think this was a good experience in cultivating this skill. This is especially so since our team is made out of full-time students, who are juggling many other responsibilities and work from other courses - hence, it is important to be both firm and understanding. + + +### CS3281 ### Frontend diff --git a/students/weiquu/observations.md b/students/weiquu/observations.md index c01fe7e3c..470b93e84 100644 --- a/students/weiquu/observations.md +++ b/students/weiquu/observations.md @@ -8,9 +8,9 @@ I have mainly worked on the improvement of visuals, adding some information on t My [first issue](https://github.com/FreezingMoon/AncientBeast/issues/2536) was to show a 'Skip turn' icon when the user hovers over an active unit. To solve this issue (PR [here](https://github.com/FreezingMoon/AncientBeast/pull/2558)), I added some assets to the assets loaded as well as an additional hint type. Then, I added the 'Skip turn' icon if the new hint type was called. -My [second issue](https://github.com/FreezingMoon/AncientBeast/issues/2537) was to show the selected ability above the hovered hexagon when targeting the ability. This issue presented a different challenge, since the hint types above are only used for units. To solve this issue (PR [here](https://github.com/FreezingMoon/AncientBeast/pull/2567) and still ongoing), I had to add the unit abilities to the assets, and then add an 'ability' class to the hovered hex's overlay visual state, removing the class as necessary. Then, I checked if the 'ability' class was present, got the appropriate ability asset, and set it to be shown above the hexagon. +My [second issue](https://github.com/FreezingMoon/AncientBeast/issues/2537) was to show the selected ability above the hovered hexagon when targeting the ability. This issue presented a different challenge, since the hint types above are only used for units. To solve this issue (PR [here](https://github.com/FreezingMoon/AncientBeast/pull/2567) and still ongoing), I had to add the unit abilities to the assets, and then add an 'ability' class to the hovered hex's overlay visual state, removing the class as necessary. Then, I checked if the 'ability' class was present, got the appropriate ability asset, and set it to be shown above the hexagon. Unfortunately, due to some complications and a decision to focus more on TEAMMATES work, I have been unable to resolve some of the problems with the PR. I plan to complete it during the exam weeks. -I plan to take on this [third issue](https://github.com/FreezingMoon/AncientBeast/issues/2539) after the PR above is merged, as it is makes use of the changes made in the above 2 PRs. +Afterwards, I plan to take on this [third issue](https://github.com/FreezingMoon/AncientBeast/issues/2539) after the PR above is merged, as it is makes use of the changes made in the above 2 PRs. ### My Learning Record diff --git a/students/weiquu/progress.md b/students/weiquu/progress.md index d96e42e0b..08019c0e5 100644 --- a/students/weiquu/progress.md +++ b/students/weiquu/progress.md @@ -1,3 +1,13 @@ +#### Summary +I mainly reviewed PRs, with a few merged PRs, in the following areas: + +- Migration of actions, their tests, and subsequent bug fixes +- Migration of E2E tests data, as well as the E2E tests themselves +- Testing for SQL injection +- Adding tests for frontend components, and improving the process for frontend tests +- Miscellanous frontend issue fixes +- Initial setup for the Multiple Course Structures feature + | Week | Achievements | | -------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | 1 | Reviewed PR: [[#12657] Instructor Home Page: Students dropdown buttons of the wrong colour . #12691](https://github.com/TEAMMATES/teammates/pull/12691) | @@ -55,3 +65,24 @@ | 7 | Reviewed PR: [[#12048] SQL Injection tests for FeedbackResponseCommentsDbIT #12853](https://github.com/TEAMMATES/teammates/pull/12853) | | 7 | Merged PR: [[#12048] Migrate GetFeedbackSessionLogsAction #12862](https://github.com/TEAMMATES/teammates/pull/12862) | | 7 | Reviewed PR: [[#12048] Migrate StudentCourseJoinConfirmationPageE2ETest #12815](https://github.com/TEAMMATES/teammates/pull/12815) | +| 7 | Reviewed PR: [Add testcases for FeedbackResponseCommentsLogicTest #12769](https://github.com/TEAMMATES/teammates/pull/12769) | +| 7 | Reviewed PR: [[#12048] Create Non Course Data Migration Script #12785](https://github.com/TEAMMATES/teammates/pull/12785) | +| 8 | Reviewed PR: [[#12048] SQL injection test for FeedbackQuestionsDbIT #12847](https://github.com/TEAMMATES/teammates/pull/12847) | +| 9 | Reviewed PR: [[#12588] Add unit test to rubric-question-edit-details-form #12907](https://github.com/TEAMMATES/teammates/pull/12907) | +| 9 | Reviewed PR: [[#12048] Add SQL injection tests in FeedbackSessionsDbIT #12857](https://github.com/TEAMMATES/teammates/pull/12857) | +| 9 | Reviewed PR: [[#12048] Migrate FeedbackMsqQuestionE2ETest #12904](https://github.com/TEAMMATES/teammates/pull/12904) | +| 9 | Reviewed PR: [[#12048] Migrate InstructorNotificationsPageE2E #12906](https://github.com/TEAMMATES/teammates/pull/12906) | +| 10 | Reviewed PR: [[#12920] Create script to migrate noSQL test data to SQL schema format #12922](https://github.com/TEAMMATES/teammates/pull/12922) | +| 10 | Created Issue: [Multiple Course Structures #12946](https://github.com/TEAMMATES/teammates/issues/12946) | +| 10 | Merged PR: [[#12946] Enable CI on Multiple Course Structures branch #12947](https://github.com/TEAMMATES/teammates/pull/12947) | +| 11 | Reviewed PR: [[#12946] Initialise Entities for Course Structure #12948](https://github.com/TEAMMATES/teammates/pull/12948) | +| 13 | Reviewed PR: [[#12946] Revert attributes removed in initialisation of MCS entities #12983](https://github.com/TEAMMATES/teammates/pull/12983) | +| 13 | Reviewed PR: [[#12048] Fix FeedBackResponsesLogic bug #13024](https://github.com/TEAMMATES/teammates/pull/13024) | +| 13 | Reviewed PR: [[#12048] Fix getSessionResultAction bugs #13023](https://github.com/TEAMMATES/teammates/pull/13023) | +| 13 | Reviewed PR: [[#12666] Instructor's Student Records Page: Moderate response button (on panel) #12964](https://github.com/TEAMMATES/teammates/pull/12964) | +| 13 | Reviewed PR: [[#12048] Migrate InstructorStudentRecordsPageE2ETest #13013](https://github.com/TEAMMATES/teammates/pull/13013) | +| 13 | Reviewed PR: [[#12048] Migrate InstructorStudentListPageE2ETest #13014](https://github.com/TEAMMATES/teammates/pull/13014) | +| 13 | Reviewed PR: [[#12048] Migrate instructor home page e2e test #13016](https://github.com/TEAMMATES/teammates/pull/13016) | +| 13 | Reviewed PR: [[#12048] Migrate StudentCourseJoinConfirmationPageE2ETest #13008](https://github.com/TEAMMATES/teammates/pull/13008) | +| 13 | Reviewed PR: [[#12048] Migrate StudentCourseDetailsPageE2ETest #13012](https://github.com/TEAMMATES/teammates/pull/13012) | +| R | Reviewed PR: [[#12048] Migrate FeedbackRubricQuestionsE2E #13078](https://github.com/TEAMMATES/teammates/pull/13078) |