Addition of Saving and Loading of Filter Presets [WIP]#4755
Addition of Saving and Loading of Filter Presets [WIP]#4755feyruzb wants to merge 5 commits intoEricsson:masterfrom
Conversation
feyruzb
added
enhancement 🌟
API change 📄
There was a problem hiding this comment.
Overall good work, I've found only minor issues (even nitpicks) which can be fixed in no time. Please check the TODOs and comments with question marks and make them clear for the future. What I'm missing are the tests for the new functions. Please check them and write tests in the corresponding unit test folders.
| export PATH="$HOME/codechecker/build/CodeChecker/bin:$PATH" | ||
|
|
||
| echo "CodeChecker rebuild completed successfully!" | ||
| echo "You can now use CodeChecker commands." No newline at end of file |
There was a problem hiding this comment.
Add a new line to the end of the file, git can complain about missing new lines at the end of any text files.
| // if the id is -1 a new preset filter is created | ||
| // the filter preset name must be unique. | ||
| // An error must be thrown if another preset exists with the same name. | ||
| // The encoding of the name must be unicode. (whitespaces allowed?) |
There was a problem hiding this comment.
In this line, "whitespaces allowed?" suggests me that this part is not yet complete. Please decide if either allowed or not and edit the comment accordingly.
| // Filter grouping api calls. | ||
| //============================================ | ||
|
|
||
| // Stores the given FilterPreset with the given id |
There was a problem hiding this comment.
I got a few questions / remarks regarding this part:
- Does it check the id of the incoming FilterPreset to decide if it exists or not? If so, please modify the first line accordingly (e.g. "If
thea preset exists with the given id, it overwrites the name, and all preset values.") - Fix the second line to match the first: "If the preset does not exist yet, it creates it with the given id.".
- The line "if the id is -1 a new preset filter is created" is confusing so if I add -1 as an id, it forcibly creates the filter? And if so, what will be the actual id after creation?
- Please make all new sentences start with a capital letter and finish with a dot.
| FilterPreset getFilterPreset(1: i64 id) | ||
| throws (1: codechecker_api_shared.RequestFailed requestError); | ||
|
|
||
| // Removes the filterpreset with the given id |
There was a problem hiding this comment.
It should be written as "FilterPreset" to be uniform to the other comments.
| description=description)) | ||
| return results | ||
|
|
||
| # Stores the given FilterPreset with the given id |
There was a problem hiding this comment.
I suggest you do the same modifications I've suggested above for the same comment.
| if preset is None: | ||
| return None | ||
|
|
||
| # convert ORM to dict for ReportFilter |
There was a problem hiding this comment.
| # convert ORM to dict for ReportFilter | |
| # Convert ORM to dict for ReportFilter |
| if not token: | ||
| return "" | ||
|
|
||
| # special case for formatting confirmed bug. IN UI for some reason |
There was a problem hiding this comment.
| # special case for formatting confirmed bug. IN UI for some reason | |
| # Special case for formatting confirmed bug. IN UI for some reason |
| session.add(preset_entry) | ||
| session.commit() | ||
| return int(preset_entry.id) | ||
| return -1 |
There was a problem hiding this comment.
Not a terrible thing but some linters may complain about using magic numbers. You could create a const for it either here or in a global file (there might be already a file for status and error codes in codechecker) which you could use as a return value. E.g. FILTER_PRESET_ERROR = -1 or similar.
| Delete preset from products list based on preset_id. | ||
| """ | ||
| self.__require_admin() | ||
| LOG.info("deleting filter preset by id") |
There was a problem hiding this comment.
| LOG.info("deleting filter preset by id") | |
| LOG.info("Deleting filter preset by ID") |
| Returns the FilterPreset identified by id. | ||
| """ | ||
| self.__require_view() | ||
| LOG.info("Returning filter Preset by id") |
There was a problem hiding this comment.
| LOG.info("Returning filter Preset by id") | |
| LOG.info("Returning filter Preset by ID") |
|
One more thing which I've missed: Besides the unit tests, please also write functional tests, for the API too. |
2 participants
This pull request introduces a new "filter preset" feature to CodeChecker, allowing users to save, retrieve, list, and delete named sets of report filters. It also includes updates to the API version and necessary database changes to support this functionality. The changes span the backend, API, database schema, and client helpers.
Filter Preset Feature:
FilterPresetstruct toreport_server.thriftand defined four new API endpoints:storeFilterPreset,getFilterPreset,deleteFilterPreset, andlistFilterPresetfor managing filter presets. Also addedgetNameByValueForFilterto convert enum values to UI-friendly names. [1] [2] [3]report_server.pyfor storing, retrieving, deleting, and listing filter presets, including validation and error handling.results.py) to call the new filter preset API endpoints.Database and Migration Updates:
filter_presetstable via Alembic migration, with unique preset names and JSON storage for report filters.FilterPresetORM model torun_db_model.pyto support the new table. [1] [2]Version and Packaging Updates:
6.68.0across Python, Node.js, and client/server package manifests. [1] [2] [3] [4] [5] [6] [7]Build and Maintenance:
completly-rebuild-thrift.shto automate rebuilding the Thrift API and related environment setup.