Skip to content

Conversation

@MorrisJobke
Copy link
Member

cc @rullzer @nickvergessen @schiessle

I will create a ticket to check our codebase for similar usages.

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews downstream labels Oct 24, 2016
@MorrisJobke MorrisJobke added this to the Nextcloud 11.0 milestone Oct 24, 2016
@mention-bot
Copy link

@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @PVince81 and @DeepDiver1975 to be potential reviewers.

@MorrisJobke
Copy link
Member Author

cc @icewind1991

@rullzer
Copy link
Member

rullzer commented Oct 25, 2016

I like it.
👍

);

$query = $queryBuilder->execute();
return (int)$query->fetchColumn();
Copy link
Member

Choose a reason for hiding this comment

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

Missing $query->closeCursor()

'appid', $queryBuilder->createNamedParameter('login'))
)
->andWhere($queryBuilder->expr()->eq(
'configkey', $queryBuilder->createNamedParameter('lastLogin'))
Copy link
Member

Choose a reason for hiding this comment

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

code style is odd in here


/**
* @param \Closure $callback
* @param string $search
Copy link
Member

Choose a reason for hiding this comment

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

Can't see this

* @return string[] with user ids
*/
private function getSeenUserIds($limit = null, $offset = null) {
$queryBuilder = \OC::$server->getDatabaseConnection()->getQueryBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

DI?

Copy link
Member

Choose a reason for hiding this comment

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

Nah that will cause the DB to be initialized to early. So then installing fails. We should fix that at some point. But probabaly can't right now.

* @since 9.2.0
*/
public function countSeenUsers() {
$queryBuilder = \OC::$server->getDatabaseConnection()->getQueryBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

DI?


while ($row = $query->fetch()) {
$result[] = $row['userid'];
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing $query->closeCursor()


return $result;
}
/**
Copy link
Member

Choose a reason for hiding this comment

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

new line before this?

* @param \Closure $callback
* @param string $search
* @since 9.2.0
*/
Copy link
Member

Choose a reason for hiding this comment

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

awkward space

* @param string $search
* @since 9.2.0
*/
public function callForSeenUsers (\Closure $callback) {
Copy link
Member

Choose a reason for hiding this comment

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

awkward space

butonic and others added 2 commits October 28, 2016 08:44
* introduce callForSeenUsers and countSeenUsers

* add tests

* oracle should support not null on clob

* since 9.2.0
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
public function countSeenUsers();

/**
* @param \Closure $callback
Copy link
Member

Choose a reason for hiding this comment

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

invalid doc

* Fixed comments

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Copy link
Member

rullzer commented Oct 28, 2016

I fixed most of the comments.
Made the IConfig always injected.

Injecting the DB here fails hard as it gets initialized to early then.

I say lets get this in. 👍

@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 28, 2016
@nickvergessen
Copy link
Member

Fine by me, also tested with the announcement center
Failing tests are sign-off only.

👍

@nickvergessen nickvergessen merged commit f2cae3c into master Oct 28, 2016
@nickvergessen nickvergessen deleted the downstream-26361 branch October 28, 2016 09:42
@MorrisJobke MorrisJobke removed their assignment Nov 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants