Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[frontend] Add page titles to Settings pages and convert to functional components #8160

Conversation

VerboseCat
Copy link
Contributor

@VerboseCat VerboseCat commented Aug 23, 2024

Proposed changes

This PR is a follow-up to the the PR which introduced custom page titles (#7374). This PR converts several 'Settings' components from classes to functional and adds page titles. These components were not included in the original PR because they were class components and could not use the page title hook.

Impacted Pages

  • Settings/Security/Groups (settings/Groups.tsx)
  • Settings/Security/Marking Definitions (settings/MarkingDefinitions.tsx)
  • Settings/Security/Roles (settings/Roles.tsx)
  • Settings/Security/Sessions (settings/Sessions.jsx)

Related issues

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case (coverage and e2e)
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

This PR has a known issue, which has also been reported in a comment on (#7753). Every time a page renders, both the the custom page title setting function useConnectedDocumentModifier.setTitle and the default page title setting function useDocumentLangModifier run and set the title. In some instances, the default page title is set after the custom page title is set. This results in the default page title being displayed. I was unable to find a cause or pattern for which components exhibit this bug.

The following components from this PR are impacted:

  • Settings: Security: Sessions

@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 2 times, most recently from 10c779e to 4f88181 Compare August 28, 2024 13:24
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 5 times, most recently from 8f8479a to cda8fe9 Compare September 6, 2024 13:46
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.96%. Comparing base (402d6bc) to head (58090f8).
Report is 15 commits behind head on release/6.5.0.

Additional details and impacted files
@@                Coverage Diff                @@
##           release/6.5.0    #8160      +/-   ##
=================================================
- Coverage          64.97%   64.96%   -0.01%     
=================================================
  Files                623      623              
  Lines              59496    59496              
  Branches            6577     6575       -2     
=================================================
- Hits               38656    38651       -5     
- Misses             20840    20845       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VerboseCat VerboseCat marked this pull request as ready for review September 6, 2024 21:26
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 2 times, most recently from a16d044 to 14658ad Compare September 10, 2024 22:54
@aHenryJard aHenryJard added the community use to identify PR from community label Sep 12, 2024
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch from 14658ad to 4eea986 Compare September 12, 2024 18:15
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 3 times, most recently from c3c5ef3 to 79df089 Compare September 26, 2024 16:09
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch from 79df089 to 4166d20 Compare October 1, 2024 12:31
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 2 times, most recently from 110452d to f917015 Compare October 17, 2024 12:02
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 2 times, most recently from d8b2642 to 7ad134d Compare October 22, 2024 14:46
@VerboseCat VerboseCat changed the base branch from master to release/6.4.0 October 30, 2024 12:14
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 3 times, most recently from 19d08d1 to 5d9806d Compare November 4, 2024 15:30
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 2 times, most recently from b141aa5 to 3d1f751 Compare November 14, 2024 13:13
@SarahBocognano SarahBocognano self-assigned this Nov 14, 2024
orderAsc: params.orderAsc !== false,
searchTerm: params.searchTerm ?? '',
view: params.view ?? 'lines',
sortBy: params.sortBy ?? 'name',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value "name" doesn't exist in marking definitions, resulting as an error in pagination. The default value for a marking is "definition"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed default sortBy to 'definition'

return (
<div className={classes.container}>
<AccessesMenu />
<Breadcrumbs elements={[{ label: t_i18n('Settings') }, { label: t_i18n('Security') }, { label: t_i18n('Sessions'), current: true }]} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Not enough margin between the Breadcrumbs and the SearchInput, then between the searchInput and the SessionList

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed margin padding to match the padding in the ListLines component which is used by the other Settings>Security components.

{t('minutes left')} /{' '}
{Math.round(userSession.originalMaxAge / 60)}
</div>
const sortByNameCaseInsensitive = R.sortBy(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not adding to much work (or code) it will be amazing to remove Ramda and use pure JS in this component :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch 2 times, most recently from 2221390 to 6df8edb Compare November 19, 2024 13:42
@VerboseCat
Copy link
Contributor Author

VerboseCat commented Nov 19, 2024

@SarahBocognano I believe all comments are resolved by the most recent commit. Thank you for your review!

@VerboseCat Awesome thank you so much for your work ! I'm gonna approuve this PR, please just check the CI as it has to be green before merging, otherwise, everything is okay !

@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch from 6df8edb to 9255afc Compare November 21, 2024 15:29
@VerboseCat VerboseCat changed the base branch from release/6.4.0 to release/6.5.0 November 21, 2024 16:22
Bonsai8863 and others added 3 commits November 26, 2024 15:33
Revert WIP conversion of Sectors

test commit

Fix paginationOptions issue for Groups, KillChainPhases, Roles

Fix MarkingDefinitions, update KillChainPhases to reflect master

Remove changes established in PR OpenCTI-Platform#7753

Add Malwares.tsx to removed changes

Post-rebase cleanup

Remove changes from PR 7753

Remove outdated changes to KillChainPhases

Fix translations

Align GroupLine design with master

Fix typing on GroupLines;

Remove variant from breadcrumbs
@VerboseCat VerboseCat force-pushed the convert_class_components_settings branch from 9255afc to 58090f8 Compare November 26, 2024 20:33
@SarahBocognano SarahBocognano merged commit 754c3f7 into OpenCTI-Platform:release/6.5.0 Nov 27, 2024
4 checks passed
Archidoit pushed a commit that referenced this pull request Dec 3, 2024
…l components (#8160)

Co-authored-by: Bonsai8863 <[email protected]>
Co-authored-by: Adrien Servel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community use to identify PR from community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants