Add default board #527

NebriBlackwing · 2018-07-11T20:22:18Z

This PR should close #197, this is my first PR into an open source project.

If I have missed anything / need revisions please advise upon completing your review.

Thanks very much, this was a great learning experience!

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

…Test Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

juliusknorr

Some small codestyle comments. But besides that, really nice work.

juliusknorr · 2018-07-12T14:19:27Z

tests/unit/Service/DefaultBoardServiceTest.php

+	}
+
+	public function testCheckFirstRunCaseTrue() {
+		$appName = "Deck";


Always use single quotes please 😉

juliusknorr · 2018-07-12T14:21:00Z

tests/unit/Service/DefaultBoardServiceTest.php

+		$this->cardService = $this->createMock(CardService::class);
+		$this->config = $this->createMock(IConfig::class);
+
+		$this->l10n = $this->request = $this->getMockBuilder(


You can also use createMock here.

juliusknorr · 2018-07-12T14:21:18Z

tests/unit/Service/DefaultBoardServiceTest.php

+	}
+
+	public function testCheckFirstRunCaseFalse() {
+		$appName = "deck";


Single quotes

juliusknorr · 2018-07-12T14:21:57Z

tests/unit/controller/PageControllerTest.php

+	public function testIndexOnFirstRun() {
+
+		$board = new Board();
+		$board->setTitle("Personal");


single quotes

juliusknorr · 2018-07-12T14:22:33Z

lib/Service/DefaultBoardService.php

+    }
+
+    public function checkFirstRun($userId, $appName) {        
+		$firstRun = $this->config->getUserValue($userId,$appName,'firstRun','yes');


Please add a space after the ,

juliusknorr · 2018-07-12T14:22:44Z

lib/Service/DefaultBoardService.php

+		$userBoards = $this->boardMapper->findAllByUser($userId);
+
+		if ($firstRun === 'yes' && count($userBoards) === 0) {
+			$this->config->setUserValue($userId,$appName,'firstRun','no');


space after ,

juliusknorr · 2018-07-12T14:23:03Z

tests/unit/controller/PageControllerTest.php


+	public function testIndexOnSecondRun() {
+
+		$this->config->setUserValue($this->userId,'deck','firstRun','no');


space after ,

juliusknorr · 2018-07-12T14:23:55Z

l10n/en_GB.js

    "View more" : "View more",
    "Move board to archive" : "Move board to archive",
-    "Create a new board" : "Create a new board"
+	"Create a new board" : "Create a new board"	


This change can be removed as well.

Isn't this string part of the l10n definitions? I see it in every other file, all I did here was remove the comma I appended at the end of the line, and obvious I accidentally added in some extra white space.

I was referring to the whitespace 😉 But that's a non-blocker.

juliusknorr · 2018-07-12T14:24:04Z

l10n/en_GB.json

    "View more" : "View more",
    "Move board to archive" : "Move board to archive",
-    "Create a new board" : "Create a new board"
+	"Create a new board" : "Create a new board"	


juliusknorr · 2018-07-12T14:45:33Z

👍 for adding tests. There seems to be one still failing:


There was 1 failure:

1) OCA\Deck\Service\DefaultBoardServiceTest::testCreateDefaultBoard
Expectation failed for method name is equal to <string:create> when invoked 3 time(s)
Parameter 0 for invocation #0 OCA\Deck\Service\StackService::create(null, 5, 1) does not match expected value.
Failed asserting that null matches expected 'To do'.

/drone/src/github.com/nextcloud/server/apps/deck/lib/Service/DefaultBoardService.php:78
/drone/src/github.com/nextcloud/server/apps/deck/tests/unit/Service/DefaultBoardServiceTest.php:168

NebriBlackwing · 2018-07-12T14:56:56Z

There was 1 failure:

OCA\Deck\Service\DefaultBoardServiceTest::testCreateDefaultBoard
Expectation failed for method name is equal to string:create when invoked 3 time(s)

Parameter 0 for invocation #0 OCA\Deck\Service\StackService::create(null, 5, 1) does not match expected value.
Failed asserting that null matches expected 'To do'.

/drone/src/github.com/nextcloud/server/apps/deck/lib/Service/DefaultBoardService.php:78
/drone/src/github.com/nextcloud/server/apps/deck/tests/unit/Service/DefaultBoardServiceTest.php:168

Part of this test is checking that the name of the stacks is being set, and those names are going through l10n->t which does not yet have a definition for those strings, hence why the test fails.

Should I be removing that part of the test?

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

juliusknorr · 2018-07-13T11:57:28Z

tests/unit/Service/DefaultBoardServiceTest.php

+			->willReturn($board);
+
+		$stackToDoId = '123';		
+		$stackToDo = $this->assembleTestStack($this->l10n->t('To do'), $stackToDoId, $boardId);		


If you mock the t call on l10n with something like:

$this->l10n->expects($this->any()) ->method('t') ->willReturnCallback(function($text) { return $text } );

you can just call $this->assembleTestStack('To do' ... then and use the plain string for further checks.

Great, just submitted the revision, once your build server finishes it's thing you should be good to go :).

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

juliusknorr · 2018-07-14T10:12:25Z

Sign off check failing due to github branch update. Let's merge this.

juliusknorr · 2018-07-14T10:13:35Z

@Nebri Congratulations to your first open source contribution. 🎉 I'm glad to have you on board for the deck app.

Ryan Fletcher added 12 commits July 10, 2018 17:10

First Attempt at implementing createDefaultBoard()

e208fd8

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

corrected null errors from first attempt.

683354d

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

first attempt at writing the checkFirstRun() in DefaultBoardService.php

a36dfcc

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Completed checkFirstRun() in DefaultBoardService.php

8a8cffc

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Added a unit test against DefaultBoardService->TestCreateDefaultBoard()

61fb68f

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Corrected missing import and missing parameter in defaultBoardService…

e07fc0a

…Test Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Corrected testCreateDefaultBoard()

712ab9e

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Updated constructor in PageControllerTest

08f9874

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Wrote unit tests for checkFirstRun method in the default board service

2002841

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Updated PageControllerTest.php to reflect new changes

be8a347

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

First attempt at using l10n

ba378ea

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Removing translations I attempted to put in before knowing the process.

af92da9

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

NebriBlackwing changed the title ~~Issue 197~~ Closes Issue #197 pending successful review. Jul 11, 2018

juliusknorr changed the title ~~Closes Issue #197 pending successful review.~~ Add default board Jul 12, 2018

juliusknorr self-requested a review July 12, 2018 06:37

juliusknorr added enhancement 3. to review labels Jul 12, 2018

juliusknorr added this to the 0.5.0 milestone Jul 12, 2018

juliusknorr requested changes Jul 12, 2018

View reviewed changes

Ryan Fletcher added 2 commits July 12, 2018 11:17

Fixed up code styles as per review by juliushaertl

8d4dbd4

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

Used $l10n references in my unit test, this should allow it pass.

3863c74

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

juliusknorr reviewed Jul 13, 2018

View reviewed changes

Mocked up $this->l10n calls in DefaultBoardServiceTest

04cb5f6

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

juliusknorr approved these changes Jul 14, 2018

View reviewed changes

Merge branch 'master' into issue_197

3163563

juliusknorr added 4. to release and removed 3. to review labels Jul 14, 2018

juliusknorr merged commit 6bf4efb into nextcloud:master Jul 14, 2018

snyk-bot mentioned this pull request Jun 12, 2023

[Snyk] Upgrade @nextcloud/auth from 2.0.0 to 2.1.0 AkshayraviC09YC47/deck#4

Open


		public function testIndexOnSecondRun() {

		$this->config->setUserValue($this->userId,'deck','firstRun','no');

Add default board #527

Add default board #527

Uh oh!

Conversation

NebriBlackwing commented Jul 11, 2018

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Jul 12, 2018

Uh oh!

NebriBlackwing commented Jul 12, 2018

Uh oh!

juliusknorr Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Jul 14, 2018

Uh oh!

juliusknorr commented Jul 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juliusknorr Jul 13, 2018 •

edited

Loading