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

Subscribed Catalogue changes to support multiple Authentication Types #737

Merged
merged 5 commits into from
Feb 7, 2023

Conversation

joe-crawford
Copy link
Contributor

@joe-crawford joe-crawford commented Jan 24, 2023

  • Display versions (or model version tags) for Published Models in the tree entry for a Subscribed Catalogue.
  • In the Subscribed Catalogue admin settings page, add fields to choose Authentication Type and required authentication fields for each type (No Authentication, API Key or OAuth (Client Credentials)).

Depends on MauroDataMapper/mdm-resources#84.

@joe-crawford joe-crawford changed the title Subscribed Catalogue changes Subscribed Catalogue changes to display Published Model version numbers and support multiple Authentication Types Jan 24, 2023
@joe-crawford joe-crawford changed the title Subscribed Catalogue changes to display Published Model version numbers and support multiple Authentication Types Subscribed Catalogue changes to support multiple Authentication Types Jan 24, 2023
@joe-crawford
Copy link
Contributor Author

Screenshots after changes:

Subscribed Catalogues list in Admin Settings (/admin/subscribedCatalogues):
Screenshot 2023-01-24 at 15 50 54

Subscribed Catalogue entry/update page (/admin/subscribedCatalogue/{{id}}):
Screenshot 2023-01-24 at 15 50 08

Subscribed Catalogue tree view:
Screenshot 2023-01-24 at 15 53 19
...

@jamesrwelch
Copy link
Contributor

One typescript error, and a couple of minor issues after merging and upgrading to latest Typescript. I'm happy to take a look myself and fix before accepting

@joe-crawford joe-crawford force-pushed the feature/mc-9890 branch 4 times, most recently from 2a25c8b to c1a9d42 Compare January 31, 2023 10:51
Copy link
Contributor

@jamesrwelch jamesrwelch left a comment

Choose a reason for hiding this comment

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

This all seems to work well, and the tests pass and the linting succeeds - good job! One minor change, please - the styling of the hint "The syndication feed URL to connect to" makes it unclear where the hint should go, or what it relates to. Are you able to add some spacing or something? This may be a more general problem with hints that we should resolve globally at a later date.
Screenshot 2023-02-01 at 06 41 46

jamesrwelch
jamesrwelch previously approved these changes Feb 3, 2023
Copy link
Contributor

@jamesrwelch jamesrwelch left a comment

Choose a reason for hiding this comment

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

Looks much better, works much better. Great work!

@jamesrwelch
Copy link
Contributor

@joe-crawford Not sure what's going on with the conflicts though 😕

@joe-crawford
Copy link
Contributor Author

joe-crawford commented Feb 3, 2023 via email

- Allow creating and updating Subscribed Catalogues with different authentication types, showing/hiding different fields
- Tweak formatting on list of Subscribed Catalogues admin component
@joe-crawford
Copy link
Contributor Author

Latest commit has a rebase and adds some padding to the field captions, and fixes an issue with the required field validation when toggling between different authentication types.

Copy link
Contributor

@jamesrwelch jamesrwelch left a comment

Choose a reason for hiding this comment

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

Really great piece of work - nice one!

@jamesrwelch jamesrwelch merged commit 5289c2c into develop Feb 7, 2023
@jamesrwelch jamesrwelch deleted the feature/mc-9890 branch February 7, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants