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: Add support for product export in UI #8281

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

sradevski
Copy link
Member

@sradevski sradevski commented Jul 25, 2024

I've keep the PR simple, so it is missing column selection (we should discuss if we event want it now or in the polish stage) and push notifications (similarly, needs discussion if we want to tackle it right away).

One thing we need to improve (outside of the scope here) is to preserve query params on handleSuccess call, so filters and such is not lost if you eg. go to create and go back.

CLOSES CC-234

@sradevski sradevski requested a review from a team as a code owner July 25, 2024 11:28
Copy link

changeset-bot bot commented Jul 25, 2024

⚠️ No Changeset found

Latest commit: 0d4099f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jul 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 29, 2024 6:31pm
6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Jul 29, 2024 6:31pm
api-reference-v2 ⬜️ Ignored (Inspect) Visit Preview Jul 29, 2024 6:31pm
docs-ui ⬜️ Ignored (Inspect) Visit Preview Jul 29, 2024 6:31pm
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Jul 29, 2024 6:31pm
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Jul 29, 2024 6:31pm
resources-docs ⬜️ Ignored (Inspect) Visit Preview Jul 29, 2024 6:31pm

onRemove={handleRemove}
readonly={readonly}
/>
{!readonly && (
Copy link
Member Author

Choose a reason for hiding this comment

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

These are just indentation changes, no actual code changed other than the readonly check.

filters,
readonly,
prefix,
}: DataTableFilterProps) => {
const { t } = useTranslation()
const [searchParams] = useSearchParams()
const [open, setOpen] = useState(false)
Copy link
Member Author

Choose a reason for hiding this comment

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

Question: Can't we rely solely on the query parameters for the activeFilters state? I feel like it should be possible, and we can remove some code around maintaining their state (so basically we have getInitialFilters be the state, and that gets memoized with location.search passed to it)

@olivermrbl
Copy link
Contributor

/snapshot-this

@olivermrbl
Copy link
Contributor

olivermrbl commented Jul 28, 2024

@sradevski, this doesn't seem to work-out-of-the box with the local file provider – did you use S3?

The exported CSV file is put into the /private folder, which is not served by the server as far as I can see. To be downloadable from admin, this should be placed in static no?

Update: Perhaps the issue is also related to the fact that the generated URL of the exported file points to a path on the local file system and not one on the server. Not sure if our Vite application can deal with this?

@sradevski
Copy link
Member Author

@olivermrbl That behavior is intentional actually. The problem with using the local file provider for the exports is that we can't have security controls (so, no support for private files) if we put it in static, as anyone would be able to read the file, so I create a private folder where private files would go (and you would need direct access to the machine to fetch them).

Let me know if you think this should behave differently, it's super easy to change it.

@olivermrbl
Copy link
Contributor

The problem with using the local file provider for the exports is that we can't have security controls (so, no support for private files) if we put it in static, as anyone would be able to read the file

Given the local file provider is only used for development, wouldn't this be OK?

@sradevski
Copy link
Member Author

The problem with using the local file provider for the exports is that we can't have security controls (so, no support for private files) if we put it in static, as anyone would be able to read the file

Given the local file provider is only used for development, wouldn't this be OK?

I've heard people use it in production as well for some small sites, so I thought to err on the safe side. If we are OK with it since people really shouldn't use it in prod, then we can def. put it in static. WDYT?

You can actually configure if you want it in static or private currently (you just need to set privateUploadDir_ to the same URL as uploadDir_), but it defaults to private. Maybe we can default to static instead?

@olivermrbl
Copy link
Contributor

You can actually configure if you want it in static or private currently (you just need to set privateUploadDir_ to the same URL as uploadDir_), but it defaults to private. Maybe we can default to static instead?

Originally (in V1), the provider was supposed to be for development only, and IMO, this should still be the recommendation from our side. With that in mind, I would suggest we optimize for this use case such that we can guarantee a good first experience across all our features.

Then in the rare (my hunch; how often have you heard this being used in prod?) case, we can guide developers to a secure configuration with the local provider.

Wdyt?

@sradevski
Copy link
Member Author

@olivermrbl it should definitely be only for development, but I've seen at least 2 conversations on Discord, and one from a local agency (and I obviously told them to use S3 😄).

Then let me use the same, static directory for private files as well, and leave a comment that if they really wanted to they can change it through the config, WDYT?

@olivermrbl
Copy link
Contributor

Then let me use the same, static directory for private files as well, and leave a comment that if they really wanted to they can change it through the config, WDYT?

Sounds good!

@olivermrbl
Copy link
Contributor

@sradevski, the static folder is correctly used now for the file uploads. However, we still have the issue with the admin dashboard attempting to download from the local file system. This leads to the download containing the index.html because that's the fallback in case the requested file cannot be found.

@olivermrbl
Copy link
Contributor

@kasperkristensen, could you give this a review? 🙏

@sradevski sradevski force-pushed the feat/add-support-export-product-ui branch from 0c20399 to b22d407 Compare July 29, 2024 17:16
@sradevski
Copy link
Member Author

@olivermrbl I missed removing one check (see last commit), should be good now.

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM!

@olivermrbl
Copy link
Contributor

@sradevski, works nicely – good one 👍

@olivermrbl olivermrbl merged commit b539c6d into develop Jul 29, 2024
23 checks passed
@olivermrbl olivermrbl deleted the feat/add-support-export-product-ui branch July 29, 2024 19:50
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.

2 participants