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

Rebuild rolescopepicker #5580

Merged
merged 21 commits into from
Nov 11, 2022
Merged

Rebuild rolescopepicker #5580

merged 21 commits into from
Nov 11, 2022

Conversation

Thingus
Copy link
Contributor

@Thingus Thingus commented Nov 4, 2022

I have:

  • Formatted any Python files with black
  • Brought the branch up to date with master permission_rework
  • Added any relevant Github labels
  • Added tests for any new additions
  • Added or updated any relevant documentation
  • Added an Architectural Decision Record (ADR), if appropriate
  • Added an MPLv2 License Header if appropriate
  • Updated the Changelog

Description

This re-implements RoleScopePicker, reimplementing CascadePicker within Flowauth. There is also a Cypress component test.
Note: This does not use any of util.js, but util.js is still used by RightsCascade, in ServerAdmin.jsx.


@Thingus Thingus added FlowAuth Issues related to FlowAuth javascript Pull requests that update Javascript code labels Nov 4, 2022
@Thingus Thingus requested review from flowstef and greenape November 4, 2022 08:51
// label={"Scopes"}
/>
);
// This mess takes every scope that is
Copy link
Member

Choose a reason for hiding this comment

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

Easier to just return that form from the backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs both fully-resolved scope paths and the grouped rep; it'd need to be mangling whatever's returned in one direction or the other.


})
Copy link
Contributor

Choose a reason for hiding this comment

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

No newline?! 😱
I thought the code formatter takes care of these things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Knew I'd forgotten to do something...

@@ -2,109 +2,227 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

/* eslint-disable react/prop-types */
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we don't disable this?
If it really makes no sense perhaps we can disable in eslint's config globally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's something we should be doing (according to eslint), but haven't - I'd like to put it in in future, but I think it adds a dependency.

Copy link
Contributor

@flowstef flowstef left a comment

Choose a reason for hiding this comment

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

Nothing horribly wrong jumps out at me.

Copy link
Member

@greenape greenape left a comment

Choose a reason for hiding this comment

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

I'm happy to merge as well I think.

@Thingus Thingus merged commit ac53f8a into permissions_rework Nov 11, 2022
@Thingus Thingus deleted the rebuild_rolescopepicker branch November 11, 2022 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FlowAuth Issues related to FlowAuth javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants