presence: add API endpoints to update presentity and notify watchers

[eschmidbauer]

No description provided.

[charlesrchance]

Is there a reason for presentity updates made in this way not to be replicated (if enable_dmq = 1)?

Also, why not re-use the existing update_presentity() function instead of creating a new function to do the same thing?

[eschmidbauer]

oops sorry i didn't think about dmq support. the API endpoint is scrictly to update the presentity database, the existing update_presentity() function seems to do a lot more than that... i will look into integrating the 2 or at least add dmq support to the API function

[charlesrchance]

No worries - the existing function will work with only a minimal set of non-null parameters and will also take care of dmq replication at the same time.

[eschmidbauer]

@charlesrchance thanks for the comment on this, turned out it was not too difficult to call the existing function. when you get a chance, please let me know what you think about this.
my tests show it's working (calling the API with some test code updates presentity table)

[charlesrchance]

Yes, looks better now 👍

The same applies to the other new function - it can be replaced by a call to existing function pres_refresh_watchers(), passing type=1 and last two parameters NULL.

[eschmidbauer]

@charlesrchance went a step further and allowed the type to get passed into pres_refresh_watchers()
another API func can be added later to allow the last 2 params if needed

[charlesrchance]

Nice...why not include the last two params now, though, for completeness?

Then it's just a question of the docs - seems to be a lot of API functions missing already, so could be a good time to bring them up to date. What do others think?

[eschmidbauer]

looking a few lines below my code, the KI interface uses a separate function if the last 2 params are required.
i dont think we need to bloat the code just for completeness-- if someone desires to use that API function they can easily add it. but for my purposes, i will not be using it and probably never will

[charlesrchance]

To me, adding a separate function for an extra two params is bloating the code.

The final two params are a direct passthrough, anyway. So your function simply becomes:

int _api_pres_refresh_watchers(str *pres, str *event, int type, str *file_uri, str *filename)
{
	return pres_refresh_watchers(pres, event, type, file_uri, filename);
}

Compared with:

int _api_pres_refresh_watchers(str *pres, str *event, int type)
{
	return pres_refresh_watchers(pres, event, type, NULL, NULL);
}

int _api_pres_refresh_watchers_file(str *pres, str *event, int type, str *file_uri, str *filename)
{
	return pres_refresh_watchers(pres, event, type, file_uri, filename);
}

Plus you consider others' needs in future and not just your own right now.

Just my opinion - perhaps others think differently.

[eschmidbauer]

the problem i see with this:

int _api_pres_refresh_watchers(str *pres, str *event, int type, str *file_uri, str *filename)
{
	return pres_refresh_watchers(pres, event, type, file_uri, filename);
}

is the last 2 parameters are optional, yet you have no way of knowing that by looking at the API function. therefore the API function becomes confusing. and another developer could spend a lot of time parsing through the code to figure out which params are required and which are optional (much like i just had to do in order to implement these 2 API functions). it will save developers a lot more time to create API functions with only required parameters

[eschmidbauer]

i am considering others' needs as Im preparing these API functions in order to submit a new module which will use them. The new module will be for everyone to freely use-- and if there becomes a need to use any more API functions, i expect the developer that requires them, will have to do exactly as I did, and create those API functions. But there is also a good chance that the API function we are talking about will never be required so IMO it becomes bloat. there is also a good chance the API functions i just created will never be used by other code except the new module im writing

[charlesrchance]

the last 2 parameters are option, yet you have no way of knowing that by looking at the API function.

Then there are, of course, the docs :)

No worries, anyway - it was just an observation - it was by no means supposed to detract from your contribution. I'll leave it for others now to comment/review further.

[eschmidbauer]

merging this since i haven't received further comment

[miconda]

3 days for the entire PR (and 2 days from last comment) is not a long period of time, specially if people are traveling or vacation.

Before merging, if it is not a module you developed, then ask again like:

If no further comments in 3 days, then I will merge ...

This is a generic remark, not saying that this PR should be reverted. But overall, it's not advisable to rush and merge specially on big changes.

[eschmidbauer]

ok, i will keep that in mind going forward

[miconda]

@eschmidbauer - also just another remark for the future - the personal branches should use userid/branch-name, not only branch-name -- e.g., eschmidbauer/presence-api. It should be written somewhere in the contribution guidelines or in the wiki for devs info. Branches without the userid that are not officially created for a specific purpose (releases, continuous integration) can be removed without notice.

[eschmidbauer]

good to know, thanks for the info