Skip to content

Conversation

@shoom3301
Copy link
Contributor

@shoom3301 shoom3301 commented May 29, 2024

@shoom3301 shoom3301 requested a review from a team May 29, 2024 09:44
@shoom3301 shoom3301 self-assigned this May 29, 2024
"dev": "strapi develop",
"start": "strapi start",
"build": "npm run build:strapi && npm run build:lib",
"prebuild:strapi": "cd src/plugins/import-notifications && yarn install && yarn run build && cd ../../..",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the official way to do it?

Maybe wrap it in parentheisis so its executed in a sub-shell, this way, if the yarn fails you don't end up in the wrong dir, and also you don't need the last cd ../..//

Copy link
Contributor

Choose a reason for hiding this comment

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

If you must go back, use cd -.

get: operations["get/faq-questions/{id}"];
put: operations["put/faq-questions/{id}"];
delete: operations["delete/faq-questions/{id}"];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, I thought I genereated this before

get: operations["get/notifications-consumer"];
put: operations["put/notifications-consumer"];
delete: operations["delete/notifications-consumer"];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Also interesting, I thought we published this too

// Uncomment to set the permissions of the plugin here
// {
// action: '', // the action name should be plugin::plugin-name.actionType
// subject: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't get it, how do we manage permissions?

const App = () => {
return (
<div>
{/*@ts-ignore*/}
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, why ts-ignore?

},
"maintainers": [
{
"name": "A Strapi developer"
Copy link
Contributor

Choose a reason for hiding this comment

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

nup :)

"styled-components": "^5.2.1"
},
"author": {
"name": "A Strapi developer"
Copy link
Contributor

Choose a reason for hiding this comment

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

nup

return {error: e.message}
}

if (!templatesMap || !notifications.length) return
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to return undefined in this case?

notifications.map(notification => {
const {templateId, account, data} = notification

return strapi.entityService.create(
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add here a small attempt to get the notification, in case the pair template, account already exists and it has the same data?

I'm thinking on an error in the middle and potential re-attempt could lead to duplicated events


const templateId = +row[0].trim()
const account = row[1].trim()
const data = parseData(row[2].trim())
Copy link
Contributor

Choose a reason for hiding this comment

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

mmm, I thought the:

  • data would be the 2nd, 3rd, 4th column, ... (while we have more columns)
  • We would use the header of the CSV to name the fields for the template

@@ -0,0 +1,17 @@
import { NotificationRaw } from './parseNotification';

const NOTIFICATIONS_KEYS: (keyof NotificationRaw)[] = ['templateId', 'account', 'data']
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the fields would account as mandatory, and the optional additional columns

The templateId I think we need to show it in the UI in the selector, and when selected would be awesome to show the preview. And after we load the CSV would be good we apply the the substitutions using the first row.

If everything goes well, the user can do the final upload

I realised this is more work, so no need to this now, but this is how I thought about this feature

…pload-notifications-csv

# Conflicts:
#	src/extensions/documentation/documentation/1.0.0/full_documentation.json
@cowprotocol cowprotocol deleted a comment from socket-security bot Jun 18, 2024
@shoom3301 shoom3301 merged commit b92d774 into main Jun 19, 2024
@alfetopito alfetopito deleted the feat/upload-notifications-csv branch June 19, 2024 12:44
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.

4 participants