Skip to content

Conversation

@sualko
Copy link
Member

@sualko sualko commented Nov 25, 2021

It's basically the same as the existing endpoint, but with a few enhancements (which could also be integrated with the existing controller):

  • no CSRF required
  • CORS support
  • versionized endpoint

cc @leonvandebroek

fix #457

Signed-off-by: sualko <[email protected]>
use OCP\IRequest;
use OCP\IUserManager;

class RecommendationApiController extends ApiController {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The first sentence in the info box is saying: "This is purely for compatibility reasons. If you are planning to offer an external API, go for a REST APIs instead.".

Copy link
Member

Choose a reason for hiding this comment

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

hmm yeah, not completely sure. I remember seeing some new stuff using OCSController. It's probably better to confirm with someone involved in Nextcloud since a longer time :)

@ChristophWurst do you know what is the recommended way?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not very experienced with external API. @nickvergessen is the expert here :)

Copy link
Member

Choose a reason for hiding this comment

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

Definitely should be an OCS controller, so all our clients can hook it up and use it in the future

Copy link
Member

Choose a reason for hiding this comment

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

Well then what is this API needed for?

Copy link
Member

Choose a reason for hiding this comment

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

As per #457 they want to use it in the Outlook plugin, so yeah that is exactly why we have OCS apis and it should be used in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I transformed it to an OCS controller, but I don't know who will maintain it in the future 😉

Copy link
Member

Choose a reason for hiding this comment

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

@sualko please document the API at https://docs.nextcloud.com/server/latest/developer_manual/client_apis/OCS/index.html.

We'll allow the API but there are no guarantees for future compatibility, hence please flag as experimental.

Copy link
Member

Choose a reason for hiding this comment

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

Also @sualko can you ping us or @ChristophWurst respectively when this is fully integrated and "stable". Reason is we would then merge it when the docs are in but wouldn't publish a new release yet, so in case you need to do some more changes during integration that could still be done and there wouldn't be any scenario that would require migration logic for app upgrades, since this wasn't published yet, which would safe everybody some time.

If this is already finished and works fine from your perspective, please let us know.

sualko added a commit to nextcloud/documentation that referenced this pull request Dec 27, 2021
for recommendations of files and folders

see nextcloud/recommendations#458

Signed-off-by: sualko <[email protected]>
@sualko
Copy link
Member Author

sualko commented Dec 27, 2021

@leonvandebroek can you please test this and report if it's working for you?

@ChristophWurst documentation is prepared (nextcloud/documentation#7877) and I submitted a pr to fix a bug which prevents the xml serialization (nextcloud/server#30417).

sualko added a commit to nextcloud/documentation that referenced this pull request Jan 7, 2022
for recommendations of files and folders

see nextcloud/recommendations#458

Signed-off-by: sualko <[email protected]>
@blizzz blizzz added this to the Nextcloud 25 milestone Apr 21, 2022
Comment on lines +34 to +42

/** @var IConfig */
private $config;

/** @var IUserManager */
private $userManager;

/** @var RecommendationService */
private $service;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @var IConfig */
private $config;
/** @var IUserManager */
private $userManager;
/** @var RecommendationService */
private $service;
private IConfig $config;
private IUserManager $userManager;
private RecommendationService $service;
private ?string $userId;

Comment on lines +44 to +46
public function __construct($appName, IRequest $request, IConfig $config,
IUserManager $userManager,
RecommendationService $service, $userId) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function __construct($appName, IRequest $request, IConfig $config,
IUserManager $userManager,
RecommendationService $service, $userId) {
public function __construct(string $appName, IRequest $request, IConfig $config,
IUserManager $userManager,
RecommendationService $service, ?string $userId) {

$this->userId = $userId;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
/**
* Get the recommendations for the user if the recommendations are enabled for the user

);
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
/**
* Get the recommendations for the user even if the recommendations are disablde for the user

@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@Pytal
Copy link
Member

Pytal commented Apr 1, 2024

Could this be rebased @sualko ?

@joshtrichards
Copy link
Member

Superseded by #829

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.

10 participants