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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 Enhancement: Make create/update email/push/sms endpoints consistent #7919

Open
2 tasks done
stnguyen90 opened this issue Apr 4, 2024 · 2 comments
Open
2 tasks done
Labels
enhancement New feature or request product / messaging Fixes and upgrades for the Appwrite Messaging.

Comments

@stnguyen90
Copy link
Contributor

stnguyen90 commented Apr 4, 2024

馃敄 Enhancement description

All the APIs should have the same order of parameters. For example, the create email endpoint:

->param('messageId', '', new CustomId(), 'Message ID. Choose a custom ID or generate a random ID with `ID.unique()`. Valid chars are a-z, A-Z, 0-9, period, hyphen, and underscore. Can\'t start with a special char. Max length is 36 chars.')
->param('subject', '', new Text(998), 'Email Subject.')
->param('content', '', new Text(64230), 'Email Content.')
->param('topics', [], new ArrayList(new UID()), 'List of Topic IDs.', true)
->param('users', [], new ArrayList(new UID()), 'List of User IDs.', true)
->param('targets', [], new ArrayList(new UID()), 'List of Targets IDs.', true)
->param('cc', [], new ArrayList(new UID()), 'Array of target IDs to be added as CC.', true)
->param('bcc', [], new ArrayList(new UID()), 'Array of target IDs to be added as BCC.', true)
->param('attachments', [], new ArrayList(new CompoundUID()), 'Array of compound ID strings of bucket IDs and file IDs to be attached to the email. They should be formatted as <BUCKET_ID>:<FILE_ID>.', true)
->param('draft', false, new Boolean(), 'Is message a draft', true)
->param('html', false, new Boolean(), 'Is content of type HTML', true)
->param('scheduledAt', null, new DatetimeValidator(requireDateInFuture: true), 'Scheduled delivery time for message in [ISO 8601](https://www.iso.org/iso-8601-date-and-time-format.html) format. DateTime value must be in future.', true)

vs update email:

->param('messageId', '', new UID(), 'Message ID.')
->param('topics', null, new ArrayList(new UID()), 'List of Topic IDs.', true)
->param('users', null, new ArrayList(new UID()), 'List of User IDs.', true)
->param('targets', null, new ArrayList(new UID()), 'List of Targets IDs.', true)
->param('subject', null, new Text(998), 'Email Subject.', true)
->param('content', null, new Text(64230), 'Email Content.', true)
->param('draft', null, new Boolean(), 'Is message a draft', true)
->param('html', null, new Boolean(), 'Is content of type HTML', true)
->param('cc', null, new ArrayList(new UID()), 'Array of target IDs to be added as CC.', true)
->param('bcc', null, new ArrayList(new UID()), 'Array of target IDs to be added as BCC.', true)
->param('scheduledAt', null, new DatetimeValidator(requireDateInFuture: true), 'Scheduled delivery time for message in [ISO 8601](https://www.iso.org/iso-8601-date-and-time-format.html) format. DateTime value must be in future.', true)

馃帳 Pitch

Having the same order will make the code more consistent and reduce confusion when calling create vs update.

It could be confusing params are out of order with the create methods, not just attachments. Let's add a task to make all the param orders consistent in the next minor verison

Originally posted by @abnegate in #7885 (comment)

馃憖 Have you spent some time to check if this issue has been raised before?

  • I checked and didn't find similar issue

馃彚 Have you read the Code of Conduct?

@stnguyen90 stnguyen90 added enhancement New feature or request product / messaging Fixes and upgrades for the Appwrite Messaging. labels Apr 4, 2024
@akash19coder
Copy link

@stnguyen90 I investigated the issue and discovered that it relates to reordering the ->param to ensure the same order in each HTTP requests? I've also noticed such inconsistent ordering in other section of the file. Correct me if I am wrong, but it seems to be a good-first-issue, off the top of my head. If that is the case, I am willing to work on this issue? It will be a good starting point for me in Appwrite community

@stnguyen90
Copy link
Contributor Author

@akash19coder, thanks for your interest! Yes, you're correct; we need to change the order of the params. However, this would cause SDKs to break as params in the SDKs would be reordered. As such, we will be doing this in 1.6.0. If you're interested in good first issues, make sure to search for issues labeled with "good first issue".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request product / messaging Fixes and upgrades for the Appwrite Messaging.
Projects
Status: Todo
Development

No branches or pull requests

2 participants