Skip to content

Conversation

@powlaa
Copy link
Contributor

@powlaa powlaa commented Jan 13, 2021

Copy link

@RespectableRuessel RespectableRuessel left a comment

Choose a reason for hiding this comment

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

⭐For instructions in the README.md on how to build your webapp for production.
(I think it's still the default README but it worked for us)
⭐ For no changes in "Files Changed" tab of the refactoring from vue-cli to create-nuxt-app. (See #1 in instructions)
⭐ ⭐ For the API connection between your front- and backend.
⭐ For an upvote button that behaves according to the authentication state of your user
⭐ For a delete and edit button that is only visible to the author of the post.
🔴 We didn't find software tests for the login / logout.

Looks good so far!

Comment on lines +17 to +23
state.token = token;
if (token) {
let { userId } = jwt_decode(token);
state.currentUser = userId;
} else {
state.currentUser = null;
}

Choose a reason for hiding this comment

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

Maybe consider to only set the token if it is valid. Otherwise (if the token contains garbage) your state will be broken because your loggedIn method checks !!state.token. So it will show True even if the token is invalid.

Comment on lines 100 to 105
it("renders a message when the item list is empty", async () => {
createComponent({
allPostsQueryHandler: jest.fn().mockResolvedValue({ data: { posts: [] } }),
});
await wrapper.vm.$nextTick();

Choose a reason for hiding this comment

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

⭐ Refactor looks good. We also used wrapper.vm.$nextTick() which is considered ugly but at least it works.

Comment on lines +5 to +8
<button v-if="loggedIn" @click="upvote">Upvote</button>
<button v-if="loggedIn" @click="downvote">Downvote</button>
<button v-if="isAuthor" @click="remove">Remove</button>
<button v-if="isAuthor" @click="edit">Edit</button>

Choose a reason for hiding this comment

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

⭐Implemented Upvote and Downvote in logged in state, Remove and Edit only if it's the author. Optional: Only show the upvote or downvote button if you didn't upvoted / downvoted before.

Copy link
Contributor

Choose a reason for hiding this comment

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

➕ 1️⃣

Copy link
Contributor

@jobegrabber jobegrabber left a comment

Choose a reason for hiding this comment

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

And

⭐ For no changes in "Files Changed" tab of the refactoring from vue-cli to create-nuxt-app. (See #1 in instructions)
⭐ ⭐ For the API connection between your front- and backend.

Comment on lines +17 to +22
### build for production and launch server

```
npm run build
npm run start
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

⭐ For instructions in the README.md on how to build your webapp for production.

@@ -0,0 +1,75 @@
import { createLocalVue, shallowMount } from "@vue/test-utils";
Copy link
Contributor

Choose a reason for hiding this comment

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

Good job!

⭐ ⭐ For a login feature in your webapp including a Vue component and its software tests.

@@ -0,0 +1,86 @@
import { createLocalVue, shallowMount, RouterLinkStub } from "@vue/test-utils";
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great,

⭐ ⭐ For a menu component which shows a login or logout button and its software tests.

it("calls logout when logout button is clicked", () => {
const logoutBtn = wrapper.find("#logoutBtn");
logoutBtn.trigger("click");
expect(logout.mock.calls.length).toBe(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(logout.mock.calls.length).toBe(1);
expect(logout).toBeCalledTimes(1);

is the idiomatic way I think

Comment on lines +5 to +8
<button v-if="loggedIn" @click="upvote">Upvote</button>
<button v-if="loggedIn" @click="downvote">Downvote</button>
<button v-if="isAuthor" @click="remove">Remove</button>
<button v-if="isAuthor" @click="edit">Edit</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

➕ 1️⃣

<button @click="upvote">Upvote</button>
<button @click="downvote">Downvote</button>
<button @click="remove">Remove</button>
<button v-if="loggedIn" @click="upvote">Upvote</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

⭐ For an upvote button that behaves according to the authentication state of your user

@jobegrabber jobegrabber self-requested a review January 26, 2021 22:32
@medizinmensch
Copy link
Contributor

Hi @Systems-Development-and-Frameworks/lichtow !

I got issues with building your app. It looks like at least one .gql file is missing?

yarn generate
yarn run v1.22.10
$ nuxt generate

 WARN  When using nuxt generate, you should set target: 'static' in your nuxt.config                                                                                     👉 Learn more about it on https://go.nuxtjs.dev/static-target

ℹ️ Production build                                                                                                                                               
ℹ️ Bundling for server and client side                                                                                                     
ℹ️ Target: static                                                                                                                                                   
✔️ Builder initialized                                                                                                                                             
✔️ Nuxt files generated                                                                                                                                       

✖️ Client
  Compiled with some errors in 9.37s

◯ Server
  


Hash: cd71a9ba8f2eea713ed0
Version: webpack 4.46.0
Time: 9370ms
Built at: 02/03/2021 12:20:32 PM
     Asset      Size  Chunks               Chunk Names
0f0f11e.js  2.32 KiB       4  [immutable]  runtime
3bfc721.js  5.13 KiB       6  [immutable]  
65e1c16.js  56.2 KiB       0  [immutable]  app
7bcc1d3.js  2.63 KiB       3  [immutable]  pages/login
  LICENSES  1.35 KiB                       
ca9c59f.js   205 KiB       1  [immutable]  commons/app
d3c8c37.js   227 KiB       5  [immutable]  vendors/app
ee66a56.js  14.8 KiB       2  [immutable]  pages/index
Entrypoint app = 0f0f11e.js ca9c59f.js d3c8c37.js 65e1c16.js

ERROR in ./store/index.js
Module not found: Error: Can't resolve '../gql/login.gql' in '/home/youri/dev/uni/sdaf/groups/lichtow/webapp/store'
 @ ./store/index.js 3:0-41 45:26-31
 @ ./.nuxt/store.js
 @ ./.nuxt/index.js
 @ ./.nuxt/client.js
 @ multi ./node_modules/@nuxt/components/lib/installComponents.js ./.nuxt/client.js

 FATAL  Nuxt build error                            

  at WebpackBundler.webpackCompile (node_modules/@nuxt/webpack/dist/webpack.js:5497:21)
  at processTicksAndRejections (node:internal/process/task_queues:94:5)
  at async WebpackBundler.build (node_modules/@nuxt/webpack/dist/webpack.js:5446:5)
  at async Builder.build (node_modules/@nuxt/builder/dist/builder.js:5634:5)
  at async Generator.initiate (node_modules/@nuxt/generator/dist/generator.js:171:7)
  at async Generator.generate (node_modules/@nuxt/generator/dist/generator.js:133:5)
  at async Object.run (node_modules/@nuxt/cli/dist/cli-generate.js:376:24)
  at async NuxtCommand.run (node_modules/@nuxt/cli/dist/cli-index.js:2803:7)


   ╭─────────────────────────────╮
   │                             │
   │   ✖️ Nuxt Fatal Error        │
   │                             │
   │   Error: Nuxt build error   │
   │                             │
   ╰─────────────────────────────╯

Same is happening with npm dev .

Could you help me out here? Do you have the login.gql?

It looks good so far. Not sure why the file is missing.

Youri

@powlaa
Copy link
Contributor Author

powlaa commented Feb 3, 2021

You're absolutely right, the file was missing. It was deleted accidentally in the last commit 😅
I added the file and it is working for me now, can you try again?

@powlaa
Copy link
Contributor Author

powlaa commented Feb 3, 2021

The import was also messed up. Not sure why we didn't realize this before. So sorry!

Copy link
Contributor

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

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

flawless

Yay, you received all ⭐ in this exercise, 13 out of 13. Excellent work.

⭐ For instructions in the README.md on how to build your webapp for production.

⭐ For no changes in "Files Changed" tab of the refactoring from vue-cli to create-nuxt-app. (See #1 in instructions)

⭐ ⭐ For the API connection between your front- and backend.

⭐ For your previous frontend tests still passing. Requests to the backend are mocked.

⭐ ⭐ For a login feature in your webapp including a Vue component and its software tests.

⭐ ⭐ For a menu component which shows a login or logout button and its software tests.

⭐ For an upvote button that behaves according to the authentication state of your user

⭐ For a delete button that is only visible to the author of the post.

⭐ For Lighthouse reporting that your production website is installable as PWA (except HTTPS).

⭐ For requesting a review and reviewing another team's PR.

See a few suggestions below

expect(submitBtn.exists()).toBe(true);
});

describe("invalid credentials", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

valid credentials would be nice.

@@ -0,0 +1,64 @@
<template>
<div>
<form onsubmit="event.preventDefault();">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<form onsubmit="event.preventDefault();">
<form @submit.prevent="submit">

type="submit"
aria-label="Login"
value="Login"
@click="submit"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have the submit on the <form> you can omit that. With the form submit, you could also submit by hitting enter.

}
},
},
data: function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data: function () {
data() {

export default {
methods: {
...mapActions(["login"]),
submit: async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
submit: async function () {
async submit() {

@@ -0,0 +1,8 @@
fragment Post on Post {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice, you discovered fragments.

authenticationType: "Bearer",

// Token name for the cookie which will be set in case of authentication
tokenName: "apollo-token",
Copy link
Contributor

Choose a reason for hiding this comment

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

It's the default, no?

Suggested change
tokenName: "apollo-token",

},
},
// Sets the authentication type for any authorized request.
authenticationType: "Bearer",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
authenticationType: "Bearer",

Default?

Comment on lines +25 to +26
<style>
</style> No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<style>
</style>

Comment on lines +2 to +9
<div>
<template v-if="loggedIn">
<button id="logoutBtn" @click="logout">Logout</button>
</template>
<template v-else>
<NuxtLink to="/login">Login</NuxtLink>
</template>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div>
<template v-if="loggedIn">
<button id="logoutBtn" @click="logout">Logout</button>
</template>
<template v-else>
<NuxtLink to="/login">Login</NuxtLink>
</template>
</div>
<div>
<button v-if="loggedIn" id="logoutBtn" @click="logout">Logout</button>
<NuxtLink v-else to="/login">Login</NuxtLink>
</div>

You could even remove the <div> if you want. An if-clause with an else is allowed as root node.

background: "#ffffff",
theme_color: "#90EE90",
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Swappshot Thu Feb 11 22:02:29 2021

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.

7 participants