Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Add the endpoints to update 1 or many versions of the app or disable all + Replacing all asserts for testing in the service tests layer + more tests + remove asserts #72

Merged
merged 1 commit into from
Mar 1, 2019

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Feb 25, 2019

Motivation

https://issues.jboss.org/browse/AEROGEAR-8356
https://issues.jboss.org/browse/AEROGEAR-8611
https://issues.jboss.org/browse/AEROGEAR-8612

What

  • Disable all versions of an app.
  • Update 1 or many versions of the app
  • Replacing all asserts for testing in the service tests layer
  • Improve and increase the tests in the layers
  • Remove the AppPostgreSQLRepository and PostgreSQLRepositor interfaces from pkg/web/apps/apps_psql_repository.go since they are not needed. The NewPostgreSQLRepository func uses the generic Repository interface.
  • Remove the unuseful mock from apps_psql_repository since it is not used.

Why

  • To attend the UI/project actions requirements
  • To attend project definitions

How

  • adding endpoints
  • adding tests
  • adding service and repository layer implementations

Verification Steps

  • Verify if it passed in all tests
  • Verify that the % of tests was increased
  • Verify that the project is no longer using asserts
  • The unit-test are covering the points, however, please feel free to test it manually as follows.

Setup project and database to do the tests.

  • Following the steps described here in the README
  • Then after starting the server, you can create the mock database
  • Use can use the [seed] file to do it. See here

Check endpoint to update all versions by POSTMAN

  • Call the endpoint GET apps in order to get a valid id
  • Call the endpoint POST /apps/:id/versions/disable with a JSON with the version struct or empty as the following examples.
{"disabledMessage": "text"}

screenshot 2019-02-28 at 19 29 33

screenshot 2019-02-28 at 19 29 11

  • Check that all versions of the app with ID were updated with the message informed and with the disable as true.
  • The endpoint should return HTTP code 200

Check endpoint to update 1 or N versions by POSTMAN

  • Call the endpoint GET apps in order to get a valid id
  • Call the endpoint GET apps/:id to get the version struct
  • Call the endpoint PUT /apps/:id/versions with a JSON with the []versions ( list of versions ).
[
    {
        "id": "9bc87235-6bcb-40ab-993c-8722d86e2201",
        "disabled": true,
        "disabledMessage": "test message"
    },
    {
        "id": "f6fe70a3-8c99-429c-8c77-a2efa7d0b458",
        "disabled": true,
        "disabledMessage": "test message"
    }
]

screenshot 2019-02-28 at 20 45 13

screenshot 2019-02-28 at 20 45 01

  • Check that all versions sent in the JSON data were updated.
  • The endpoint should return HTTP code 200

Checklist:

  • Code has been tested locally by PR requester
  • Changes have been successfully verified by another team member

Progress

  • Finished task

Additional Notes

@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-8356 branch 3 times, most recently from aa9464d to 595323a Compare February 25, 2019 15:14
@camilamacedo86 camilamacedo86 changed the title initial WIP: initial Feb 25, 2019
@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-8356 branch 13 times, most recently from b5dd0b4 to a9114a0 Compare February 26, 2019 12:32
@camilamacedo86 camilamacedo86 changed the title WIP: initial Implementation across all layers in order to attend the need to disable versions Feb 26, 2019
@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-8356 branch 2 times, most recently from 793a5bd to af89145 Compare February 26, 2019 13:17
@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-8356 branch 2 times, most recently from 8aff3fe to cb0c581 Compare February 26, 2019 13:28
Copy link
Member

@davidffrench davidffrench left a comment

Choose a reason for hiding this comment

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

Looks good to me Camila. One small comment.

I haven't got a chance to verify, just code review for now. If no one else verifies, I will get a chance later.

pkg/web/apps/apps_psql_repository.go Outdated Show resolved Hide resolved
Copy link

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

POST /apps/:id/versions/batch-disable

Should this be a PUT? Also a small suggestion - I think you could just have "disable" instead of "batch-disable" as not including a version ID in the path implies that all versions will be disabled. So it would look like:

PUT /apps/:id/versions/disable

@camilamacedo86
Copy link
Contributor Author

Hi @craicoverflow,

I agree with your suggestions. It was defined by @davidffrench so I will let it up to him.
Please, let us know if you find any you and/or you were able to verify.

@davidffrench
Copy link
Member

POST /apps/:id/versions/batch-disable

Should this be a PUT? Also a small suggestion - I think you could just have "disable" instead of "batch-disable" as not including a version ID in the path implies that all versions will be disabled. So it would look like:

PUT /apps/:id/versions/disable

Since it is not following REST naming standards, the consensus online was to use a POST for these type of endpoints. I am also not overly comfortable with the name batch-disable or disable. If anyone has a better more generic name for it, i am happy to hear it.

…end the need to disable all versions and/or 1 to N of them (AEROGEAR-8356)
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Feb 28, 2019

@davidffrench I prefer to use "disable" as suggested by @craicoverflow . Since we have a no better name wdyt over move forward with this one and after if you would like then it can be changed as you wish?

@camilamacedo86 camilamacedo86 requested review from craicoverflow and austincunningham and removed request for craicoverflow February 28, 2019 20:48
@craicoverflow
Copy link

POST /apps/:id/versions/batch-disable
Should this be a PUT? Also a small suggestion - I think you could just have "disable" instead of "batch-disable" as not including a version ID in the path implies that all versions will be disabled. So it would look like:
PUT /apps/:id/versions/disable

Since it is not following REST naming standards, the consensus online was to use a POST for these type of endpoints. I am also not overly comfortable with the name batch-disable or disable. If anyone has a better more generic name for it, i am happy to hear it.

What do you dislike about "disable". How about "disable-all"?

@davidffrench
Copy link
Member

@camilamacedo86 @craicoverflow disable is perfectly fine with me. Please proceed with this

Copy link
Member

@austincunningham austincunningham left a comment

Choose a reason for hiding this comment

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

I have verified , Code changes look good so I am happy to approve

@camilamacedo86
Copy link
Contributor Author

@austincunningham, @davidffrench and @craicoverflow
Really thank you for your review :-)

@camilamacedo86 camilamacedo86 merged commit 7637c04 into aerogear:master Mar 1, 2019
@camilamacedo86 camilamacedo86 deleted the AEROGEAR-8356 branch March 1, 2019 09:40
jhellar pushed a commit to jhellar/mobile-security-service that referenced this pull request Nov 13, 2019
…rogear#72)

feat(reconcile): added a flag to skip validation of the namespace
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants