Exercise 7 by ubiquitousbyte · Pull Request #15 · Systems-Development-and-Frameworks/bgrakia

ubiquitousbyte · 2021-01-12T19:34:49Z

Pull request Group CountryRoads

…, delete, posts, added login/logout functionality

aloparev

Looks good to me. You apparently did not implement the edit button, but since your delete works, it is not a big loss. And you even implemented the optional storybooks. However, I think the following files could be skipped from review: webapp/layouts/default.vue, webapp/test/NewsForm.test.js. Total result for objectives: 12/13 ⭐.

roschaefer

This is an intermediate PR. I haven't tested out your code, just looked at the source code. To me, it looks as if you already rock GraphQL, NuxtJS, VueJS and Vuetify. Congrats

roschaefer · 2021-01-25T21:52:52Z

+      const mutation = gql`
+       mutation ($title: ID!) {
+          upvote(title: $title) {
+              votes


Request the ID here and let vue-apollo do all the work to update the Post object! 💪

roschaefer · 2021-01-25T21:53:45Z

+          mutation,
+          variables: { title: this.newsItem.title }
+        })
+        this.$emit('updateNews', {...this.newsItem, votes: upvote.votes});


You could remove all code to update things that have been previously cached. vue-apollo will do it for you.

roschaefer · 2021-01-25T21:55:42Z

+      const variables = { post: { title } };
+      try {
+          const { data: { write } } = await this.$apollo.mutate({ mutation, variables })
+          this.newsItems.push({ title: write.title, votes: write.votes, author: write.author});


Use update hook to update the cached posts query: https://apollo.vuejs.org/guide/apollo/mutations.html

roschaefer · 2021-01-25T21:58:49Z

+    async getNews() {
+      const query = gql`
+        query {
+            posts {
+                title
+                votes
+                author {
+                  id
+                }
+            }
+        }
+      `;
+      try {
+        const { data: { posts }} = await this.$apollo.query( { query } );
+        return posts;
+      }
+      catch (e) {
+        return [];
+      }
+    }


Suggested change

async getNews() {

const query = gql`

query {

posts {

title

votes

author {

id

}

}

}

`;

try {

const { data: { posts }} = await this.$apollo.query( { query } );

return posts;

}

catch (e) {

return [];

}

}

apollo: {

posts: gql`

query {

posts {

title

votes

author {

id

}

}

}

`

}

Will automatically populate this.posts for you! 💪

https://apollo.vuejs.org/guide/apollo/#queries

roschaefer · 2021-01-25T21:59:49Z

+  async mounted() {
+    this.newsItems = await this.getNews();
+  },


Suggested change

async mounted() {

this.newsItems = await this.getNews();

},

Using queries according to the docs will work even in SSR. This mounted hook only runs client-side.

roschaefer · 2021-01-25T22:02:49Z

+    const [emailExists, emailIsNotTooLong] = wrapper.vm.nameRules
+
+    expect(emailExists(email)).toBe(true);
+    expect(emailIsNotTooLong(email)).toBe("User name cannot exceed 64 chars!");


Why do you call it User name if it's the email? 🤔

roschaefer · 2021-01-25T22:04:24Z

+  it('keeps the login button disabled if the email is too long', async () => {
+    const email = 'testtesttesttesttesttesttesttesttesttesttesttesttesttesttesttesttesttest'
+    const submitButton = wrapper.find('#login');
+    const [emailExists, emailIsNotTooLong] = wrapper.vm.nameRules


Consider wrapper.vm as private. If you want to unit test your validations, export them in your component:

<script> export const validations = ... export default { data() { validations, // ... } } </script>

roschaefer · 2021-01-25T22:05:28Z


 describe('NewsForm', () => {
 	let wrapper;
-	


You could have merged whitespace changes to keep the PR diff small.

roschaefer · 2021-01-25T22:06:57Z

-      expect(paragraph.text()).toEqual("The list is empty :(");
-      wrapper.destroy();
+    expect(paragraph.text()).toEqual("The list is empty :(");
+    testWrapper.destroy();


Why is it necessary to call testWrapper.destroy()?

roschaefer · 2021-01-25T22:08:33Z

+    }, shallowMount)
+    await testWrapper.vm.addItem('Title_3');
+    expect(writeMutateMock).toBeCalled()
+    expect(queryMock).toHaveBeenCalledTimes(2); // After writing, we also expect vue to rerender the component and therefore query the news again


Ideally, this is not necessary. Once the write mutation completes, it adds the new post to the cached posts query, thus triggering a re-render without unnecessary graphql queries.

medizinmensch · 2021-02-10T16:28:31Z

Hi Team!

Let's summarize first:

⭐ 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 and edit 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.

As it was correctly pointed out by @aloparev, you did not implement an edit button. that is very unfortunate as it basically only one line missing. The exercise stated that you did not have to implement the functionality behind it (neither front- nor backend!).

But it's still 12/13 ⭐ which is great!

Two more things:

I like your Readme! 👌
You get many many warnings in your tests. If this was an actual job, I would have let you fix that first 😅

ubiquitousbyte added 13 commits January 10, 2021 13:09

added and tested storybook functionality

4e2da46

added login form and page

915c38c

added state storage handling for tokens

d0c57dc

added delete functionality in the backend

768b930

added plugin for authentication handling, api calls for upvote, write…

8b5492a

…, delete, posts, added login/logout functionality

added tests for form and item with store and mutation mocking

140d234

removed redundant global constants

1b79863

refactored tests, made props be data

75fbc25

comment removed

2d9f1a0

added proper name

642e37a

added login tests

268b116

menu tests added

ee22481

add pwa config and readme

56767f9

ubiquitousbyte requested a review from roschaefer January 12, 2021 19:37

MaykAkifovski requested review from a team, aloparev and ilonae and removed request for roschaefer January 18, 2021 16:46

aloparev approved these changes Jan 23, 2021

View reviewed changes

yiimnta mentioned this pull request Jan 25, 2021

Exercise 7 Systems-Development-and-Frameworks/CountryRoads#47

Merged

roschaefer requested changes Jan 25, 2021

View reviewed changes

Conversation

ubiquitousbyte commented Jan 12, 2021 • edited by MaykAkifovski Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aloparev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roschaefer 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!

medizinmensch commented Feb 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ubiquitousbyte commented Jan 12, 2021 •

edited by MaykAkifovski

Loading