-
-
Notifications
You must be signed in to change notification settings - Fork 529
Dashboard improv: add button to filter for the user's org only #2028
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
Dashboard improv: add button to filter for the user's org only #2028
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2028 +/- ##
===========================================
+ Coverage 66.75% 75.60% +8.84%
===========================================
Files 95 482 +387
Lines 3706 14461 +10755
Branches 519 1502 +983
===========================================
+ Hits 2474 10933 +8459
- Misses 941 2930 +1989
- Partials 291 598 +307
... and 291 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
be94769 to
d5aff07
Compare
|
@mlodic its almost done now. But the problem is the response is cached so if I change the users the previous response is shown not the new one. |
|
hello @shivam-Purohit, thank you for your PR! :)
when you say "if I change the users", you mean if you logout and then login with another user?
Do you mean something like that? This how we switch the visualization in the "Job Result" page. It could make sense to leverage the same component also here. Maybe you can call the 2 options: "Org / Global" and make it "toggable" only if the user is part of an org. Otherwise it could be hidden or disabled. Thoughts? |
mlodic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is some duplicated code that must be addressed
api_app/views.py
Outdated
| if user: | ||
| filter_kwargs["user"] = user | ||
| if users: | ||
| filter_kwargs["user__in"] = users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can optimize the code by choosing a single parameter "users" here and change the filter based on the number of items in the list
api_app/views.py
Outdated
| if organization: | ||
| return self.__aggregation_response_dynamic( | ||
| "md5", False, users=users_of_organization | ||
| ) | ||
| else: | ||
| return self.__aggregation_response_dynamic("md5", False, user=current_user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should call the aggregation method a single time, we need to optimize this code
api_app/views.py
Outdated
| try: | ||
| if current_user.has_membership(): | ||
| organization = current_user.membership.organization | ||
| if organization: | ||
| users_of_organization = [ | ||
| membership.user for membership in organization.members.all() | ||
| ] | ||
|
|
||
| except AttributeError: | ||
| # Handle the case where the user has no membership | ||
| logger.info("User has no membership.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code has been replicated lots of time. We need it just once
| /* eslint-disable */ | ||
| var sendURL = JOB_AGG_STATUS_URI; | ||
| const porp = props.myprop; | ||
| const getValue = porp.key; | ||
| console.debug("props is ", getValue); | ||
| /* eslint-enable */ | ||
|
|
||
| if (getValue) { | ||
| /* eslint-disable */ | ||
| sendURL = JOB_AGG_STATUS_URI_ORG; | ||
| console.debug("this is sending to my urls", sendURL); | ||
| /* eslint-enable */ | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more generally, I don't think it is necessary to create new API endpoints for this case. We should use the URL parameters to customize the results. This would avoid a lot of additional code that would be really difficult to maintain.
Example:
aggregate/status -> like it is right now
aggregate/status?org=True -> with the toggle enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking the same. I just tried some sub-optimised code to check the core functionality. I will improve the code quality.
yes same. To check / test the feature I created two more users and added one to my org. Though when i change the users i.e logout and login again I get the results from the previous user. But if we restart the container the new results do re-render. Either it caches the result or there is some bug in my code? |
yes this look better. I will try that. But to disable the option if user is not part of any org we will need to send request to the backend during the rendering of dashboard. |
thanks! i will take a look |
|
hey @mlodic hope the Christmas went well! |
|
this is how it looks. I have to change some layout to fit in new structure. If I just move the |
|
hey, I hope this message finds you well :) I saw the images, that feels good! |
mlodic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool to me, do you want to make a little review @drosetti too?
api_app/views.py
Outdated
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger = logging.getLogger("__name__") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it while I was trying to add a console logger. I was having some problem with log file to see the recent changes. I will revert it back.
api_app/views.py
Outdated
| org_param = request.GET.get("org", "").lower() == "true" | ||
| users_of_organization = None | ||
| if org_param: | ||
| users_of_organization = self.get_org_members(request.user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be a method returning users_of_organization, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes returns all the members in the organisation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that those 4 lines could be a method which returns users_of_organization.
I am telling you this because I saw the code repeated multiple times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shivam-Purohit onceyou get some time, if you could just do this little refactor then we can merge this PR :) thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry i missed the comment above earlier. I think the new refactor should be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you did a good job, I requested little changes (most of them on the names of the const/var). Be careful with eslint-disable, I know sometimes it's annoying, but it's useful to have same code style with many developers, it should be disabled only in rare cases.
Edit: you can ask to me to review the pr after the changes. In every case if you have some doubts or questions, ask me :D
| {/* <div className="g-0 d-flex align-items-baseline flex-column flex-lg-row mb-2"> | ||
| <h3 className="fw-bold" id="Dashboard_title"> | ||
| Dashboard | ||
| </h3> | ||
| <ButtonGroup className="ms-2 mb-3 d-flex align-items-center"> | ||
| <Button | ||
| outline={!state} | ||
| color={state ? "primary" : "tertiary"} | ||
| onClick={() => handleChange()} | ||
| > | ||
| GLOBAL | ||
| </Button> | ||
| <Button | ||
| outline={state} | ||
| color={!state ? "primary" : "tertiary"} | ||
| onClick={() => handleChange()} | ||
| > | ||
| ORG | ||
| </Button> | ||
| </ButtonGroup> | ||
| <ElasticTimePicker | ||
| className="ms-auto" | ||
| size="sm" | ||
| defaultSelected={range} | ||
| onChange={onTimeIntervalChange} | ||
| id="Dashboard_timepicker" | ||
| /> | ||
| </div> */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this comment
| <ButtonGroup className="mb-3"> | ||
| <Button | ||
| outline={orgstate} | ||
| color={!orgstate ? "primary" : "tertiary"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to reverse the check, i think it's easier to understand:
orgstate ? "tertiary" : "primary"
| // const isSelectedUI = JobResultSections.VISUALIZER; | ||
| const { guideState, setGuideState } = useGuideContext(); | ||
|
|
||
| const [orgstate, setorgState] = useState(() => false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a very silly suggestion but I'd to have orgState, it has the same style of guideState (it's strange the linter doesn't mark this)
| /* eslint-enable */ | ||
|
|
||
| /* eslint-disable */ | ||
| sendURL = `${JOB_AGG_STATUS_URI}?org=${getValue}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls use a more specific name for this const, what do you think about ORG_JOB_AGG_STATUS_URI ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure!
|
@drosetti I have made the changes requested. One more thing I noticed is in the |
|
@shivam-Purohit I think this problem in the dashboard is related to the fact that the regex doesn't allow IPv6, then they are classified as generic and not show in the dashboard charts. Matteo told me you previously changed the regex to accept IPv6, I think I deleted it during a refactor, my apologies. However, I resolved and restored your work on the regex: in the develop branch you can find the refex that accept both IPv4 and IPv6, with a rebase everything should work, let me know if you have some problems. I also check your code and approved your pr from a frontend prospective, good work 💪 |
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
|
@drosetti Thanks! I think we should be done here. Do let me know if further changes are required. edit : there was some prettier error after the rebase in |
Signed-off-by: Shivam Purohit <[email protected]>
246f9be to
688ba47
Compare
Signed-off-by: Shivam Purohit <[email protected]>
* Using correct pipeline to manage visualizers (#2044) * Using correct pipeline to manage visualizers Signed-off-by: 0ssigeno <[email protected]> * Fix tests Signed-off-by: 0ssigeno <[email protected]> --------- Signed-off-by: 0ssigeno <[email protected]> * tweaks to docs * added exception catcher for Cymru analyzer * fixed bug in info column in the plugin section * fixed bug in multiple observable analysis (#2047) * fixed bug in multiple observable analysis: all observable had the type of the first one * linter * restore IP regex to accept also IPv6 * removed env in the toolbar (#2050) * removed env in the toolbar * linter * Dashboard improv: add button to filter for the user's org only (#2028) * frontend : add switch for user's org Signed-off-by: Shivam Purohit <[email protected]> * backend:add endpoint for the req Signed-off-by: Shivam Purohit <[email protected]> * initial backend url Signed-off-by: Shivam Purohit <[email protected]> * improve frontend logic Signed-off-by: Shivam Purohit <[email protected]> * add user org filter Signed-off-by: Shivam Purohit <[email protected]> * fix : try except block Signed-off-by: Shivam Purohit <[email protected]> * remove redundant url endpoints Signed-off-by: Shivam Purohit <[email protected]> * remove unnecessary commented code Signed-off-by: Shivam Purohit <[email protected]> * add hide component logic Signed-off-by: Shivam Purohit <[email protected]> * rename props Signed-off-by: Shivam Purohit <[email protected]> * reduce redundant code via functions Signed-off-by: Shivam Purohit <[email protected]> * convert get_org_members to static Signed-off-by: Shivam Purohit <[email protected]> * remove logger changes Signed-off-by: Shivam Purohit <[email protected]> * remove comments and rename variables Signed-off-by: Shivam Purohit <[email protected]> * remove console statement Signed-off-by: Shivam Purohit <[email protected]> * refactor get_org_members Signed-off-by: Shivam Purohit <[email protected]> * fix:prettier Signed-off-by: Shivam Purohit <[email protected]> --------- Signed-off-by: Shivam Purohit <[email protected]> * adjusted README * Added check for path Signed-off-by: 0ssigeno <[email protected]> * Fix corner case for multiple playbook Signed-off-by: 0ssigeno <[email protected]> * Fix pointers Signed-off-by: 0ssigeno <[email protected]> * Backend always decide the classification Signed-off-by: 0ssigeno <[email protected]> * Job bi (#2052) * job bi Signed-off-by: 0ssigeno <[email protected]> * More Signed-off-by: 0ssigeno <[email protected]> * Job bi Signed-off-by: 0ssigeno <[email protected]> * Migration Signed-off-by: 0ssigeno <[email protected]> * Fix Signed-off-by: 0ssigeno <[email protected]> * Fix Signed-off-by: 0ssigeno <[email protected]> * Added playbook in the elastic template Signed-off-by: 0ssigeno <[email protected]> * Fix Signed-off-by: 0ssigeno <[email protected]> * Missin migrations Signed-off-by: 0ssigeno <[email protected]> --------- Signed-off-by: 0ssigeno <[email protected]> * Bump pillow from 10.0.1 to 10.2.0 in /requirements (#2055) Bumps [pillow](https://github.com/python-pillow/Pillow) from 10.0.1 to 10.2.0. - [Release notes](https://github.com/python-pillow/Pillow/releases) - [Changelog](https://github.com/python-pillow/Pillow/blob/main/CHANGES.rst) - [Commits](python-pillow/Pillow@10.0.1...10.2.0) --- updated-dependencies: - dependency-name: pillow dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump checkdmarc from 4.8.4 to 5.2.7 in /requirements (#2054) Bumps [checkdmarc](https://github.com/domainaware/checkdmarc) from 4.8.4 to 5.2.7. - [Changelog](https://github.com/domainaware/checkdmarc/blob/master/CHANGELOG.md) - [Commits](https://github.com/domainaware/checkdmarc/commits) --- updated-dependencies: - dependency-name: checkdmarc dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump quark-engine from 23.9.1 to 23.12.1 in /requirements (#2045) Bumps [quark-engine](https://github.com/quark-engine/quark-engine) from 23.9.1 to 23.12.1. - [Release notes](https://github.com/quark-engine/quark-engine/releases) - [Commits](ev-flow/quark-engine@v23.9.1...v23.12.1) --- updated-dependencies: - dependency-name: quark-engine dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump flake8 from 6.1.0 to 7.0.0 in /requirements (#2056) Bumps [flake8](https://github.com/pycqa/flake8) from 6.1.0 to 7.0.0. - [Commits](PyCQA/flake8@6.1.0...7.0.0) --- updated-dependencies: - dependency-name: flake8 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Frontend - fixed runtime config bugs (#2064) * fixed runtime config bugs * restored old comment --------- Co-authored-by: Daniele Rosetti <[email protected]> * updated frontend dependencies * Refactoring of start.py script. Closes #1899 and #1866 (#2060) * Removed initialize.sh root execution * Remove sudo command from installation * Added python3-venv installation * Dropped support for docker compose V1 * Added redirection to stderr * Dropped support for docker compose V1 * Fixed download directory of script * Changed shebang for better compatibility * First not finished bash script * Formatting fix * Added parameter checks * Added other parameters check * Fixed wrongly used check * Removed start() function * Used exact string matching for parameters * Added mandatory argument parsing * Moved declaration of array inside of logical function * Added main parameter case * Added help function * Added default version to help * Completed help function * Added argument parsing * Added help option * Fixed subtle error with bash logic values * Most of the work done * Working docker cmd * Added project directory specification * Removed unused function * Improved error message * Removed reduntand if condition * Added checks for empty strings * Removed pycharm warnings * Fixed IntelOwl version * Fixed some bugs * Fixed wrong for index * Fixed git checkout * Removed wrongly placed comment * Removed python pre-requirements depencency * Added execution of initialize.sh from start * Entering venv * Removed unused echo * Added check for docker group * Updated docs to use new startup script * Fixed typo in doc * Changed generic code blocks to bash * Add curl dependency * Add curl dependency * Re added pre-requirements for compatibility * Removed old occurrences of start.py * Added test docker file for integrations * Added manual usage to doc * Removed python3 and venv dependency * Updated major release informations * Removed python and venv dependency for start * Removed python and venv dependency for start from docs * Made code-review-doctor happy * Improved installation doc * Added absolute path for application_restart * Updated docs * added deprecation notice --------- Co-authored-by: Matteo Lodi <[email protected]> * Fixed creating a playbook with an existing tag (#2057) * changed playbook serializer * deepsource --------- Co-authored-by: Matteo Lodi <[email protected]> * bump and changelog --------- Signed-off-by: 0ssigeno <[email protected]> Signed-off-by: Shivam Purohit <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Simone Berni <[email protected]> Co-authored-by: Daniele Rosetti <[email protected]> Co-authored-by: Daniele Rosetti <[email protected]> Co-authored-by: Shivam Purohit <[email protected]> Co-authored-by: 0ssigeno <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Martina Carella <[email protected]> Co-authored-by: fgibertoni <[email protected]>








Button to switch between org only reports or all the reports
closes #1936
Description
Type of change
Checklist
develop_monkeypatch()was used in its class to apply the necessary decorators.dumpplugincommand and added it in the project as a data migration. [Doc]("How to create a Plugin")test_files.zipand you added the default tests for that mimetype in test_classes.py.FREE_TO_USE_ANALYZERSplaybook inplaybook_config.json.Black,Flake,Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.testsfolder). All the tests (new and old ones) gave 0 errors.Important Rules