-
Notifications
You must be signed in to change notification settings - Fork 1
Update specification to include attachments and rate limiting #7
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
base: main
Are you sure you want to change the base?
Conversation
c5f3808
to
4073bb6
Compare
Co-authored-by: Tjaž Eržen <tjaz@codeplain.ai>
817f249
to
8f4d516
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better :). Providing complete context and avoid "guessing" is key to developing scalable integrations with plain. In some places, I saw missing context, and it's very important to address this before rendering.
- If "event_type" equals "EXTRACTION_DATA_START" or "EXTRACTION_DATA_CONTINUE" The Extraction Function should push the 'tasks' and 'users' data. To push the 'tasks' data, it should: | ||
- If `TheExtractionStateObject["tasks"]["completed"]=false`: | ||
- While Fetching The Fetched Tasks using pagination (The Tasks Iteration): | ||
- When fetching The Fetched Tasks use the query param 'fields', where the value is an array of strings, which contains at least 'hasAttachments'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still have to be adjusted to comment made in the previous review iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think this is described correctly - it has to at least contain 'hasAttachments', can also contain other values. Depends on the use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I limit it here to contain only 'hasAttachments' it will be in conflict with other FRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you phrase which contains at least 'hasAttachments'. less ambiguously as we discussed, and render. I understand you got a conflict earlier. I think we should get to the root cause of the problem why a conflict exists, and then resolve it.
8f4d516
to
7bf1bef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright 👍
Let's finish it tomorrow, but here are the things you can do right away:
- final touches for
mappings/
folder - Minor updates to
test_data/
folder
Also, I'm missing the updates for the new templating:
- every file from
mappings/
folder - Correct templating of
templates/chef_cli_normalization_validation.plain
indevrev-wrike-snapin.plain
- Type of field author_id is an array with max_length 1, which can be used directly. | ||
- task_id (display name: "Task ID", is required, type: reference) | ||
- Field task_id refers to the record type "#record:tasks". | ||
- Type of field task_id is an array with max_length 1, which can be used directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be used directly
Let's be less ambiguous - if this is only option, just say "should be used directly"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just adopt this directly from Trello.
- text (display name: "Text", is required, type: rich text) | ||
- author_id (display name: "Author ID", is required, type: reference) | ||
- Field author_id refers to the record type "#record:users". | ||
- Type of field author_id is an array with max_length 1, which can be used directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
- permalink (display name: "URL", is required, type: text) | ||
- responsible_ids (display name: "Responsible IDs", is required, type: reference) | ||
- Field responsible_ids refers to the record type "#record:users". | ||
- Type of field responsible_ids is an array with max_length 1, which can be used as array value. No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here ("can be used")
"external_system_id": "test-external_system_id", | ||
"external_system_name": "Wrike", | ||
"external_system_type": "ADaaS", | ||
"import_slug": "trello-wrike-devrev", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove Trello reference
"key": "test-key", | ||
"key_type": "", | ||
"org_id": "test-org-id", | ||
"org_name": "My FOlder" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "key" and "org_id" are not in the form we want them to be.
"key": "test-connection-key", | ||
"org_id": "test-org-id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
- If "event_type" equals "EXTRACTION_DATA_START" or "EXTRACTION_DATA_CONTINUE" The Extraction Function should push the 'tasks' and 'users' data. To push the 'tasks' data, it should: | ||
- If `TheExtractionStateObject["tasks"]["completed"]=false`: | ||
- While Fetching The Fetched Tasks using pagination (The Tasks Iteration): | ||
- When fetching The Fetched Tasks use the query param 'fields', where the value is an array of strings, which contains at least 'hasAttachments'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you phrase which contains at least 'hasAttachments'. less ambiguously as we discussed, and render. I understand you got a conflict earlier. I think we should get to the root cause of the problem why a conflict exists, and then resolve it.
No description provided.