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

Return the ID of a created appointment from createAppointment #84

Merged
merged 3 commits into from
Oct 26, 2018

Conversation

Faultless
Copy link
Contributor

Companion PR of zm-x-web's #449.

This is needed because for Jitsi/OnlyOffice integrations, we require the inviteId generated upon creation of an appointment, in order to update that appointment on the spot.

@@ -2010,6 +2010,11 @@ type SMimeMessage {
content: String
}

type createAppointmentResponse {
Copy link
Member

Choose a reason for hiding this comment

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

instead of creating new type just for two keys, we should use existing type available for appointments (CalendarItemHitInfo)

Copy link
Member

@silentsakky silentsakky left a comment

Choose a reason for hiding this comment

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

Please use existing graphql type

@Faultless
Copy link
Contributor Author

Updated, should be fine now

@Faultless
Copy link
Contributor Author

@silentsakky sorry for the ping, but if there's no other issue with this PR, perhaps you can merge it as I don't have write access?

@silentsakky
Copy link
Member

@Faultless Sure I can merge but there is small review comment pending, would be good if you can handle that as well
#84 (comment)

@Faultless
Copy link
Contributor Author

Sorry for the multiple pings, but its done @silentsakky

@Faultless
Copy link
Contributor Author

can any of the code owners merge this if there's no further changes required?

@silentsakky
Copy link
Member

@Faultless I thought we only merge changes when feature is complete and everything is ready to be merged in develop, I am not seeing corresponding changes of zm-x-web repo, also it would be better to link your changes with ZX ticket to better track progress of the entire work

@Faultless
Copy link
Contributor Author

My PR had a lot of interlap with Celina's work, and so we've rebased my work onto hers', and now the corresponding PR for these changes is here.
The thing is, I need these changes released for everyone to be able to test on Netlify. I have tried giving the same branch name on both projects to have netlify deploy them as a bundle, but it did not work.

@Faultless
Copy link
Contributor Author

That being said, this PR is minimal and has no breaking changes, so it should be safe to deploy.
Also, I am most certainly missing something on how I should go about linking work done on this project with its corresponding code on zm-x-web, but I have no reference to look at.

@silentsakky
Copy link
Member

ok for now I am merging the changes, but you can follow below process to make sure netlify deploys work and you would be able to test changes before merging anything

  • make sure branch that you create in ZimbraOS:zm-x-web corresponds to jira ticket number like ZX-123
  • make same branch exists in Zimbra:zm-api-js-client
  • this will not work with forked repos, you need to create branch in the main repo only

@silentsakky silentsakky merged commit 8007b76 into Zimbra:develop Oct 26, 2018
@silentsakky
Copy link
Member

Changes has been pushed with version 9.0.0-beta.2

@Faultless
Copy link
Contributor Author

Thank you @silentsakky !

@silentsakky
Copy link
Member

Sorry guys, I had to revert this as this is breaking our stuff currently, this will fail zm-x-web as corresponding changes in query file is missing

@silentsakky
Copy link
Member

Created new PR #90

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.

2 participants