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

feat(OY2-26538): package action changelog #290

Merged
merged 16 commits into from
Jan 10, 2024
Merged

feat(OY2-26538): package action changelog #290

merged 16 commits into from
Jan 10, 2024

Conversation

pkim-gswell
Copy link
Contributor

@pkim-gswell pkim-gswell commented Jan 2, 2024

Purpose

There are many changes in this PR. The primary change is a suggestive reorganizing of our types structure. With the new addition of opensearch changelog index, a couple pain-points came to light. First, there's a tight coupling between the core type structures and the main index. Second, a lack of division between domains has led to an inconsistent and hard-to-establish naming convention with unwieldy imports:

import type {
  OsQueryState,
  OsFilterable,
  OsAggQuery,
  OsMainSearchResponse,
} from "shared-types";

type Props = {
  filters: OsFilterable[]
  sort?: OsQueryState["sort"];
  pagination: OsQueryState["pagination"];
  aggs?: OsAggQuery[];
  data: OsMainSearchResponse
}

"All to say"

The new proposed structure cuts down on imports by taking advantage of namespaces:

import { opensearch } from "shared-types";

type Props = {
  filters: opensearch.Filterable[]
  sort?: opensearch.QueryState["sort"];
  pagination: opensearch.QueryState["pagination"];
  aggs?: opensearch.Agg["query"];
  data: opensearch.main.Response
};

In addition, the api integration of the aforementioned changelog index has been implemented.

There are 2 ways to access a package's changelog:

  1. using the useChangelogSearch hook
const { mutateAsync } = useChangelogSearch();

await mutateAsync({
  index: "changelog",
  pagination: { number: 0, size: 200 },
  filters: [
    {
      field: "packageId.keyword",
      type: "term",
      value: "CO-2444-23",
      prefix: "must",
    },
  ],
});
  1. using the useGetItem hook
const { data: item } = useGetItem(id!);

Linked Issues to Close

https://qmacbis.atlassian.net/browse/OY2-26538

Approach

Assorted Notes/Considerations/Learning

List any other information that you think is important... a post-merge activity, someone to notify, what you learned, etc.

@pkim-gswell pkim-gswell self-assigned this Jan 5, 2024
Comment on lines +1 to +3
export * as changelog from "./changelog";
export * as main from "./main";
export * from "./_";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where namespaces get defined

@@ -4,7 +4,7 @@ export * from "./errors";
export * from "./seatool";
export * from "./onemac";
export * from "./onemacLegacy";
export * from "./opensearch";
export * as opensearch from "./opensearch";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This creates the overarching opensearch namespace

@@ -0,0 +1,82 @@
export type Hit<T> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here are the opensearch core type definitions

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it. Any reason why the filename is _?

Copy link
Contributor Author

@pkim-gswell pkim-gswell Jan 9, 2024

Choose a reason for hiding this comment

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

Initially named it base.ts. But I figured that was too based.

Comment on lines +34 to +42
export type Response = Res<Document>;
export type ItemResult = Hit<Document> & {
found: boolean;
};

export type Field = keyof Document | `${keyof Document}.keyword`;
export type Filterable = FIL<Field>;
export type State = QueryState<Field>;
export type Aggs = AggQuery<Field>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both main and changelog Indices enforce the same naming convention

from: 0,
size: 200,
// NOTE: get the required timestamp sort field
sort: [{}],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdial89f

Still need to flesh out the timestamp/dates

"respond-to-rai",
];

export const PackageActivities: FC<opensearch.main.Document> = (props) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entry point for PackageActivity section


export const ACTIONS_ADMIN = ["disable-rai-withdraw", "enable-rai-withdraw"];

export const AdminChanges: FC<opensearch.main.Document> = (props) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entry point for AdminChanges section

@pkim-gswell
Copy link
Contributor Author

@mdial89f @13bfrancis @kevinhaube

Here are the remaining todos:

  • download all export feature
  • what are the correct timestamps/dates?
  • nail down the correct actions for each section

Since other tickets are blocked by this package changelog PR. I think we should move forward the merge (barring no breaking changes), and document remaining todos onto the Jira ticket.

Copy link

@hannasage hannasage left a comment

Choose a reason for hiding this comment

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

Most my questions are pattern-based so they're universal across AC and PA code

@@ -107,7 +107,7 @@ functions:
osDomain: ${param:osDomain}
events:
- http:
path: /search
path: /search/{index}

Choose a reason for hiding this comment

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

🔥 Love that we're keeping the endpoint dynamic params pattern from Actions. Keeps endpoints uniform!


return {
...main,
_source: { ...main._source, changelog: changelog.hits.hits },

Choose a reason for hiding this comment

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

Wondering if we should bundle them here or break them out as individual getters. With React components, we can easily load "per-component" rather than piping a top-level object thru to sub-components.

A quick cost-benefit analysis doesn't really raise any immediate needs tho. Just starting the convo!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. I've pulled out the changelog getter into a separate file and moved data aggregation to the handler 👍

Comment on lines +44 to +45
case "update":
return ["SPA ID update", AC_Update];

Choose a reason for hiding this comment

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

Future stuff? I know OneMAC supports this from the back-end via a lamda, so I figured it was coming soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this will be refined in the future. At the moment, we're not 100% sure of the event shape.

return useMemo(() => {
switch (doc.actionType) {
case "disable-rai-withdraw":
return ["Disabled formal RAI response withdraw", AC_WithdrawDisabled];

Choose a reason for hiding this comment

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

I like bundling these, but would a typed response be of any benefit here? I see we're doing an inline type below on line 59.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a response type 👍

@@ -133,6 +133,8 @@ export const deleteIndex: Handler = async () => {
throw "process.env.osDomain cannot be undefined";
}
await os.deleteIndex(process.env.osDomain, "main");
await os.deleteIndex(process.env.osDomain, "changelog");
await os.deleteIndex(process.env.osDomain, "seatool");
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -133,6 +133,8 @@ export const deleteIndex: Handler = async () => {
throw "process.env.osDomain cannot be undefined";
}
await os.deleteIndex(process.env.osDomain, "main");
await os.deleteIndex(process.env.osDomain, "changelog");
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -53,12 +53,12 @@ async function manageIndex() {
try {
await manageIndexResource({
index: "main",
// TODO: remove after rai transform
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, 100

Copy link
Contributor

@mdial89f mdial89f left a comment

Choose a reason for hiding this comment

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

@pkim-gswell this appears non breaking and i think this sets things up nicely. I agree with the suggestion of moving ahead with the merge. Approved from my chair, I know kevin has outstanding comments that may need addressed.
I'd really like to get rid of the transform logic as a fast follower to this PR, since the existing RAI list component and its underlying data is no longer needed; I can clean that up or assist.

@pkim-gswell pkim-gswell merged commit 8c53dd8 into master Jan 10, 2024
15 of 16 checks passed
Copy link
Contributor

🎉 This PR is included in version 1.5.0-val.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants