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-26082): Package actions FE #317

Merged
merged 14 commits into from
Jan 19, 2024
Merged

feat(OY2-26082): Package actions FE #317

merged 14 commits into from
Jan 19, 2024

Conversation

pkim-gswell
Copy link
Contributor

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

Purpose

This ticket is part of the FE piece to the Package Activity + Admin Package changelog epic.
The initial Package Activity and Admin Package section views were started in the previous ticket.

Included in this PR are the multi-attachment "download all" functionality.

Linked Issues to Close

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

Approach

In the initial planning phase, I identified two possible solution paths for download all.

  1. BE heavy approach: offloading the file aggregation and zip to a new endpoint
  2. FE focused: utilizing the existing getAttachments endpoint and installing external dependencies to enable the FE for zipping.

I went with the latter approach. With our existing systems, FE dedicated responsibility was more practical and required less additional code.

new dependencies added:

Assorted Notes/Considerations/Learning

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

type Attachments = NonNullable<opensearch.changelog.Document["attachments"]>;

export const useAttachmentService = (
props: Pick<opensearch.changelog.Document, "packageId">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is some redundancy between the prop and the function signatures within the hook.

The main reason this requires the top level changelog document is for access to the packageId. In addition, it provided consistent props scaffolding.

Comment on lines 12 to 30
const [loading, setLoading] = useState(false);
const [error, setError] = useState("");

const onUrl = async (ATT: Attachments[number]) => {
setLoading(true);
try {
return await getAttachmentUrl(
props.packageId,
ATT.bucket,
ATT.key,
ATT.filename
);
} catch (e) {
const err = e instanceof Error ? e.message : "Failed download";
setError(err);
} finally {
setLoading(false);
}
};

Choose a reason for hiding this comment

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

Can we utilize react-query for this, and pass the export thru this hook?

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 completely overlooked that... great point out!
Makes it much cleaner 🤟

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.

I'm stepping through the env checking AC.
I've noticed one so far
"The Administrative Package Changes should only show if at least one applicable update has been entered"
Section is showing even when empty. Will comment if I find others

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.

Based on AC 5.b, I think you should drop the timezone annotation on each activity title.
So the GMT -5 bit.
Personally I don't mind it. If you feel it's helpful, we can see about AC variance. As is though, I think the AC is specifying it not be present.

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.

AC 5.d: "The default expansion state of the most recent item is expanded, all other items default to collapsed."
I'm seeing everything collapsed by default.

@mdial89f
Copy link
Contributor

Based on AC 5.b, I think you should drop the timezone annotation on each activity title. So the GMT -5 bit. Personally I don't mind it. If you feel it's helpful, we can see about AC variance. As is though, I think the AC is specifying it not be present.

Official guidance is to remove the GMT -X bit.

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 It looks like the fixes are in, and this all looks great.

@pkim-gswell pkim-gswell merged commit f2a859d into master Jan 19, 2024
16 checks passed
@pkim-gswell pkim-gswell added the type: FEAT Submit new features label Jan 24, 2024
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants