added complete oauth #160

Moneexa · 2023-11-19T18:43:41Z

No description provided.

xJREB · 2023-11-27T08:56:36Z

Thank you, @Moneexa ! I had a brief first look and this seems very promising and close to mergeable. A few things I noticed:

The README needs to be updated to reflect the changes with running the E2E tests locally and the backend setup
package.json and src/plugins/api.js still contain references to Pizzly. This PR should completely remove Pizzly from the project without losing any functionality. But maybe it's as simple as deleting the references by now?
package.json also contains the dependencies @auth0/auth0-spa-js and @octokit/rest in addition to firebase. Are these two really necessary, especially since most HTTP requests seem to be sent with axios?
Parts of the new code, e.g., ConnectToGitHubButton.vue, don't seem to follow the style conventions of the project (see .editorconfig and .prettierrc.json or best use IDE support to automatically format and fix the code according to the conventions). There also seems to be code that is commented out in some places. Best remove this if it is no longer required for testing.
In the E2E tests, I would extract the URL (https://api.github.com/graphql) to a variable in a single location, e.g., in cypress\support\e2e.js. The same is true for the API base path (https://api.github.com/) in api.js, but this one should probably be extracted to something like src\config.js.
Regarding the API requests in api.js, it looks like basically every request explicitly adds the Bearer token in the header. This is fairly brittle if we want to change this in the future. I would probably use a central request interceptor for this that adds the token for the right conditions (see, e.g., https://axios-http.com/docs/interceptors)
It looks like the test repo in the E2E tests has been changed. We specifically created https://github.com/adr/adr-test-repository-empty for this, so it would be best to keep this. I will give you access.

xJREB

Thank you! I had another look and made more detailed comments on certain lines. In general, this has improved, but there is also still a lot of commented code and style guide violations (e.g., single quotes instead of double quotes, missing semicolons, etc.).

xJREB · 2023-12-04T13:41:19Z

cypress/e2e/adrManagerTest/AddRepo.cy.js


-        cy.intercept("GET", "**/user/repos**").as("getRepos");
+        // cy.intercept("GET", "**/user/repos**").as("getRepos");


Why is this commented out? If it is not needed, please delete it.

xJREB · 2023-12-04T13:42:06Z

cypress/e2e/adrManagerTest/DeleteAdr.cy.js

        cy.get("[data-cy=addRepo]").click();
        cy.wait("@getRepos").its("response.statusCode").should("eq", 200);
        cy.get("[data-cy=listRepo]").contains("ADR-Manager").click();
        cy.get("[data-cy=addRepoDialog]").click();
-        cy.intercept("GET", "**/repos**").as("showRepos");
+        // cy.intercept('POST', 'https://api.github.com/graphql').as("showRepos");


Is this still needed?

xJREB · 2023-12-04T13:42:32Z

cypress/e2e/adrManagerTest/DeleteAdr.cy.js

@@ -30,7 +34,8 @@ context("Deleting an ADR from a repo", () => {
                const addedRepos = JSON.parse(localStorage.getItem("addedRepositories"));
                expect(addedRepos[0].adrs.length).to.eq(adrCount - 1);
                expect(addedRepos[0].deletedAdrs.length).to.eq(1);
+                // });


What is this?

xJREB · 2023-12-04T13:42:46Z

cypress/e2e/adrManagerTest/DeleteRepo.cy.js

        cy.visit(TEST_BASE_URL);

        // add ADR Manager repo
-        cy.intercept("GET", "**/user/repos**").as("getRepos");
+        // cy.intercept("GET", "**/user/repos**").as("getRepos");


xJREB · 2023-12-04T13:44:22Z

cypress/e2e/adrManagerTest/PushNewAdr.cy.js

+        // cy.wait("@getCommitSha");
+        // cy.wait("@commitRequest")
+
+        // .then((val) => {
+        //     cy.log(val.request);
+        //     for (let item in val.request) {
+        //         cy.log(item);
+        //     }
+        //     cy.log(val.request.body.author);
+        // });


What about this part? Was this only for debugging? If it is generally useful for this test, then it probably should not be commented out. If it is not needed, then delete it.

xJREB · 2023-12-04T13:50:13Z

src/plugins/api.js

+                        searchResults.push(repo);
+                    }
+                });
+                console.log("#### search results ####", searchResults)


Is this really needed? Looks like temporary logging for debugging.

xJREB · 2023-12-04T13:54:04Z

tests/constants.js

I think this file is just too excessively long with so much test data. I would reduce this, as it doesn't add much value after a certain length, but makes everything less maintainable and efficient.

xJREB · 2023-12-04T13:54:41Z

tests/api-searchRepositoryList.test.js

@@ -7,18 +7,360 @@ global.localStorage = {
        return "abc....";
    }
 };
+console.log("###### print local storage #######", localStorage.getItem("authId"))


Is this still needed?

xJREB · 2023-12-04T13:54:57Z

tests/api-searchRepositoryList.test.js


    test("Test Searching Repos with list in parameter. Searching for " + searchTerm, async () => {
+        console.log(localStorage.getItem("authId"))


Is this needed?

xJREB · 2023-12-04T13:56:06Z

tests/api-searchRepositoryList.test.js

+                "data": {
+                    "user": {
+                        "repositories": {
+                            "nodes": [


These are probably also too many entries. I would reduce this as well to keep it manageable.

xJREB

General comment: if host is changed to localhost instead of 127.0.0.1 in vite.config.js, we prevent the problem with the 401 if someone follows the displayed link from vite after starting the project.

xJREB · 2023-12-05T08:49:11Z

package.json

@@ -17,6 +17,7 @@
        "antlr4": "^4.13.0",
        "axios": "^1.4.0",
        "core-js": "^3.31.1",
+        "firebase": "^10.7.0",
        "marked": "^5.1.1",
        "pizzly-js": "^0.2.8",


Shouldn't Pizzly be gone by now?

Moneexa added 2 commits November 19, 2023 19:42

added complete oauth

2a72223

fixed unit test

397a676

Moneexa assigned xJREB Nov 21, 2023

Moneexa added 5 commits November 21, 2023 18:13

fixed tests

3ff38e1

added user secret

1d6eab7

replaced secrets in cypress

eeddfba

new changes in test

3de5194

new changes in test

4521776

Moneexa added 6 commits November 30, 2023 16:01

fixed pizzly replacements

918b1e6

tests fixed

be65909

fixed interceptor

4f6fa48

fixed axios interceptor

f6ba8f8

fixed axios

1c019e9

fixed headers error

f655d27

xJREB reviewed Dec 4, 2023

View reviewed changes

Moneexa added 4 commits December 5, 2023 00:17

fixed linting and errors

24bdb10

fixed jest-diff error

88b2473

fixed dependency

0362d92

fixed test errors

572b674

xJREB reviewed Dec 5, 2023

View reviewed changes

xJREB added 2 commits December 5, 2023 11:26

Style guide formatting; removed pizzly dependency; changed host for vite

b122384

Bumped version number

63496e7

xJREB merged commit 2717d3d into main Dec 5, 2023
2 checks passed

xJREB deleted the Issue-#156 branch December 5, 2023 10:40

xJREB mentioned this pull request Jan 10, 2024

Get rid of pizzly #156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added complete oauth #160

added complete oauth #160

Moneexa commented Nov 19, 2023

xJREB commented Nov 27, 2023 •

edited

Loading

xJREB left a comment

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB Dec 4, 2023

xJREB left a comment

xJREB Dec 5, 2023


		cy.intercept("GET", "/user/repos").as("getRepos");
		// cy.intercept("GET", "/user/repos").as("getRepos");


		test("Test Searching Repos with list in parameter. Searching for " + searchTerm, async () => {
		console.log(localStorage.getItem("authId"))

added complete oauth #160

added complete oauth #160

Conversation

Moneexa commented Nov 19, 2023

xJREB commented Nov 27, 2023 • edited Loading

xJREB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xJREB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xJREB commented Nov 27, 2023 •

edited

Loading