Skip to content

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Nov 13, 2020

@PVince81
Copy link
Member

A nice start...

Nextcloud was successfully installed
An exception occurred while executing 'INSERT INTO "oc_talk_commands" ("app", "command", "name", "script", "response", "enabled") VALUES(?, ?, ?, ?, ?, ?)' with params ["", "help", "talk", "", 1, 3]:

ORA-01400: cannot insert NULL into ("AUTOTEST"."oc_talk_commands"."script")

@nickvergessen
Copy link
Member Author

nickvergessen commented Nov 16, 2020

  • Missing "update" step for the help command

@nickvergessen nickvergessen self-assigned this Dec 9, 2020
@nickvergessen nickvergessen force-pushed the techdebt/noid/tests-against-oracle branch 3 times, most recently from d0eac10 to 46a01b2 Compare December 11, 2020 10:32
@nickvergessen nickvergessen force-pushed the techdebt/noid/tests-against-oracle branch from 46a01b2 to 3d564cd Compare December 15, 2020 13:11
@nickvergessen nickvergessen marked this pull request as ready for review December 15, 2020 13:11
@nickvergessen nickvergessen force-pushed the techdebt/noid/tests-against-oracle branch from 3d564cd to 7067907 Compare December 15, 2020 14:30
@nickvergessen
Copy link
Member Author

Rebased to add "listable" column on the select

use OCP\DB\QueryBuilder\IQueryBuilder;

class SelectHelper {
public function selectRoomsTable(IQueryBuilder $query, string $alias = 'r'): void {
Copy link
Member

Choose a reason for hiding this comment

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

for readability on the calling side, I think I wouldn't make the alias optional.
I came here to check as I was confused where the alias comes from

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 alias is optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you would n't.

Well it's r for room, a for attendee and s for session. Your IDE (if you have one) will also tell you that. Not a huge fan of adding hundreds of '(a|r|s) in the code just for lazy non-IDE users

Copy link
Member

Choose a reason for hiding this comment

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

it's not only for non-IDE but also when reading the code on Github for example, like when reviewing code changes or viewing a commit / diff on Github or CLI

but as you like, not a big issue to infer the alias

Signed-off-by: Joas Schilling <[email protected]>
@PVince81
Copy link
Member

I have the same composer problem with the oci pipeline, not sure when this started happening: https://github.com/nextcloud/guests/pull/511/checks?check_run_id=1558214699

@nickvergessen
Copy link
Member Author

We use dev-master of christophwurst nextcloud package and the was accidentally broken 🙃

@nickvergessen
Copy link
Member Author

Okay actually the problem was we first installed composer deps and then set the php version, so it ran composer with 8.0 before switching to 7.4
Nevertheless this led to the relevant fix in the dependency so 8.0 will work in the future.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit b7727af into master Dec 16, 2020
@PVince81 PVince81 deleted the techdebt/noid/tests-against-oracle branch December 16, 2020 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants