refactor: settings v2 (BREAKING CHANGE) #282
Merged
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
kwajiehao
force-pushed
the
refactor/settings-v2
branch
4 times, most recently
from
61c7b50 to
6fcd39d
Compare
kwajiehao
changed the title
alexanderleegs
previously requested changes
Sep 13, 2021
There was a problem hiding this comment.
The use of fixtures is a great idea to help with unit testing, thanks for suggesting this implementation!
The auth middleware is currently missing the new routes - we should also add them in.
Aside from the other comments, I was wondering whether it would make sense to split up some of the logic in the settings route into a service level settings class, since currently other route layer classes only process the input and delegate the actual logic to the service classes called - could we discuss this more offline?
kwajiehao
force-pushed
the
refactor/settings-v2
branch
2 times, most recently
from
5ea5000 to
5f09ac8
Compare
alexanderleegs
force-pushed
the
refactor/settings-v2
branch
from
2c9bae8 to
b27b314
Compare
To modify our settings page, we need to introduce new file services for files which we read and write settings data from: - _config.yml - _data/footer.yml - _data/navigation.yml - index.md (homepage) The NavYmlService already exists so we don't need to create a file service for _data/navigation.yml.
This commit adds the V2 SettingsRouter class with an implementation of the readSettingsPage route. The update route will be implemented in a subsequent commit.
This commit extracts the settings file retrieval logic into a separate method on the SettingsRouter class. This is because the file retrieval logic is needed for both the read and update methods of the SettingsRouter.
This commit refactors the update route for Settings. The refactor simplifies the code structure by eliminating the use of an intermediary "update" object which tracks which files need to be updated. Using the new file services allows us to eliminate the need for formatting the content before posting to GitHub.
This commit refactors the readSettingsPage response to mirror the UpdateSettingsRequestSchema. This commit also refactors the readSettingsPage route function to use util functions to extract the required fields for the response.
This commit completes the test for the readSettingsPage route function. It also updates the fixtures to have the expected response from each of the page services.
…ngPage This commit updates the settings request schema to allow empty string values, and assigning required values correctly. It also fixes a logic error in checking whether or not to update the homepage when making a change to the title.
This commit completes the unit tests for the updateSettingsPage route function.
This commit adds the unit tests for the "read" and "update" methods of the HomepagePageService. It also adds the necessary fixtures for the homepage.
This commit adds the unit tests for the "read" and "update" methods of the ConfigYmlService. It also adds the necessary fixtures for the config file.
This commit adds the unit tests for the "read" and "update" methods of the FooterYmlService. It also adds the necessary fixtures for the footer file.
This commit adds the unit tests for the "read" and "update" methods of the NavYmlService. It also adds the necessary fixtures for the navigation file.
In PR 280, we added a description field to the Settings endpoints (see #280). This commit adds this field to the Settings V2 router as well.
This commit refactors the new Settings route to use a new SettingsService so that business logic in the routes layer is simplified - the details of HOW the Settings files are updated should not be exposed on the route layer. In doing so, we created a new configServices directory to store the SettingsService, since it does not exist as a file service.
This commit adds the necessary tests for the SettingsService, most of which were copied over and refactored from the route tests. Another change made was to use the toHaveBeenLastCalledWith method instead of the toHaveBeenCalledWith method, since it more accurately reflects the change we want to test.
This commit refactors the Settings route tests so that we only mock the SettingsService class and not the underlying file services that we had used before. This commit also refactors the existing SettingsService utility methods to be static methods so that we can more easily test our code.
This commit modifies how we wait for a function to be completed. Previously, we would use await expect(func), but this caused some issues where not all method calls within the function were awaited properly - by changing this to await expect(func).resolves.not.toThrow(), this problem is resolved.
alexanderleegs
force-pushed
the
refactor/settings-v2
branch
from
310d45e to
7d825b7
Compare
harishv7
pushed a commit
that referenced
this pull request
Feb 17, 2023
* feat: add new file services for Settings To modify our settings page, we need to introduce new file services for files which we read and write settings data from: - _config.yml - _data/footer.yml - _data/navigation.yml - index.md (homepage) The NavYmlService already exists so we don't need to create a file service for _data/navigation.yml. * feat: add new Settings router This commit adds the V2 SettingsRouter class with an implementation of the readSettingsPage route. The update route will be implemented in a subsequent commit. * refactor: extract settings files retrieval to method This commit extracts the settings file retrieval logic into a separate method on the SettingsRouter class. This is because the file retrieval logic is needed for both the read and update methods of the SettingsRouter. * refactor: update settings page This commit refactors the update route for Settings. The refactor simplifies the code structure by eliminating the use of an intermediary "update" object which tracks which files need to be updated. Using the new file services allows us to eliminate the need for formatting the content before posting to GitHub. * chore: import v2 settings router * refactor: add settings update schema * test: add sample data for tests * refactor: readSettingsPage response schema This commit refactors the readSettingsPage response to mirror the UpdateSettingsRequestSchema. This commit also refactors the readSettingsPage route function to use util functions to extract the required fields for the response. * chore: add URL attribute back to settings response * test: readSettingsPage This commit completes the test for the readSettingsPage route function. It also updates the fixtures to have the expected response from each of the page services. * chore: update settings request schema, fix logic error in updateSettingPage This commit updates the settings request schema to allow empty string values, and assigning required values correctly. It also fixes a logic error in checking whether or not to update the homepage when making a change to the title. * refactor: simplify footer update code * test: updateSettingsPage This commit completes the unit tests for the updateSettingsPage route function. * test: HomepagePageService unit tests This commit adds the unit tests for the "read" and "update" methods of the HomepagePageService. It also adds the necessary fixtures for the homepage. * test: ConfigYmlService unit tests This commit adds the unit tests for the "read" and "update" methods of the ConfigYmlService. It also adds the necessary fixtures for the config file. * test: FooterYmlService unit tests This commit adds the unit tests for the "read" and "update" methods of the FooterYmlService. It also adds the necessary fixtures for the footer file. * test: NavYmlService unit tests This commit adds the unit tests for the "read" and "update" methods of the NavYmlService. It also adds the necessary fixtures for the navigation file. * chore: add v2 settings route to auth middleware * chore: add description to Settings v2 In PR 280, we added a description field to the Settings endpoints (see #280). This commit adds this field to the Settings V2 router as well. * refactor: create new configService category This commit refactors the new Settings route to use a new SettingsService so that business logic in the routes layer is simplified - the details of HOW the Settings files are updated should not be exposed on the route layer. In doing so, we created a new configServices directory to store the SettingsService, since it does not exist as a file service. * chore: update config fixtures with description field * test: add tests for SettingsService This commit adds the necessary tests for the SettingsService, most of which were copied over and refactored from the route tests. Another change made was to use the toHaveBeenLastCalledWith method instead of the toHaveBeenCalledWith method, since it more accurately reflects the change we want to test. * test: refactor Settings route tests This commit refactors the Settings route tests so that we only mock the SettingsService class and not the underlying file services that we had used before. This commit also refactors the existing SettingsService utility methods to be static methods so that we can more easily test our code. * chore: typo * Fix: wait for expected method to fully resolve This commit modifies how we wait for a function to be completed. Previously, we would use await expect(func), but this caused some issues where not all method calls within the function were awaited properly - by changing this to await expect(func).resolves.not.toThrow(), this problem is resolved. * Fix: rebase errors * Fix: update schema to allow telegram and tiktok * fix: rebase errors * Refactor: change format of settings object * Fix: update test Co-authored-by: Alexander Lee <alexander@open.gov.sg>
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.
Note: this PR contains a BREAKING CHANGE.
Overview
This PR continues our efforts to refactor the CMS as specified by this design document (link). In this PR, I refactor the existing endpoints for reading and updating our repo settings:
/{siteName}/settings- read settings/{siteName}/settings- update settingsThe main change is the introduction of the new
SettingsRouterclass. TheSettingsRouteruses the newSettingsServiceclass which contains the bulk of the implementation logic, and uses the newly-createdConfigYmlService,FooterYmlService,NavYmlService, andHomepagePageServiceto simplify the retrieval and updating of settings data from different source files. There is some code duplication between the different service classes, but @alexanderleegs and I decided that any further simplification is out of the scope of this PR.This PR adds a validation schema for the request body of the
updateSettingsPageroute function, and refactors the response sent from thegetSettingsPageroute function to match this schema. This is a BREAKING CHANGE and so this PR is to be reviewed in conjunction with PR #618 on the frontend.Finally, this PR also includes tests for the newly-introduced components in the same style as the existing tests. Some changes to testing include:
fixturesdirectory which contains example test data that is reusable across different teststoHaveBeenLastCalledWithinstead oftoHaveBeenCalledWith, so that we can make more accurate assertions (this is done in addition to thejest.clearAllMocks()invocation that is done in between tests)