Feat: PUT and GET organization and tests #103

mtreacy002 · 2020-08-10T23:44:17Z

Description

Create PUT and GET /organization API endpoints

Fixes #105

Type of Change:

Code

Code/Quality Assurance Only

New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Run and test the application manually:

Login as user who already has additional_info in the database and is_organization_rep is True
try to send PUT /organization request before setting Cookie user_id through GET /organization. see error 403 Forbidden and error message

send GET /organization request. Since this user has not created an organization, see error 404 Not Found and message Organization does not exist.

now create organization by sending PUT /organization request with correct payload. see code 201 Created and success message

now try to update the organization by changing some details. On correct update, see code 200 and success message

logout and login as a user that either has additional_info is_organization_rep is False or has not created an additional_info.
send GET /organization request. see error 403 Forbidden and error message user has not declared is_organization_rep value

an example of successful GET /organization request will be as below

Run unittests and ensure all tests passed

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
Any dependent changes have been merged

Code/Quality Assurance Only

My changes generate no new warnings
My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Additional Info
This is still a work in progress

mtreacy002 · 2020-08-10T23:47:40Z

Update @anitab-org/bridgeintech-maintainers . This PR is still a WIP. At the moment it addresses GET /organization with responses 403 Forbidden when the user either did not declare that they represent an organization or they haven't created an additional information instance to state whether or not they represents an organisation.

mtreacy002 · 2020-08-10T23:59:09Z

@ramitsawhney27 . Can you please check this PR and give me feedback whether or not my approach on this GET /organization make sense. Thanks

mtreacy002 · 2020-08-11T00:03:44Z

@ramitsawhney27, I specifically want to note that I send an HTTP request to GET /user to backend within the logic of GET /organization since to get the organization that the logged-in user represents, I need to pass the user_id as parameter in the database query.

mtreacy002 · 2020-08-11T11:41:31Z

Update @anitab-org/bridgeintech-maintainers . I've completed the GET and PUT API endpoints of this PR. I still need to do the test cases though. Will force push it once I'm done. @ramitsawhney27 , can you please open an issue for this functionalities (GET and PUT /organizations) so I can link this PR to it. Thanks

ramitsawhney27 · 2020-08-11T11:50:13Z

@ramitsawhney27 . Can you please check this PR and give me feedback whether or not my approach on this GET /organization make sense. Thanks

This is fine

mtreacy002 · 2020-08-12T04:59:44Z

Update @anitab-org/bridgeintech-maintainers . This PR is now completed. Please review when you have time. Thanks

meenakshi-dhanani · 2020-08-12T05:10:05Z

app/api/dao/organization.py

+            The OrganizationModel class represented by the user whose ID was searched.
+        """
+
+        rep_additional_info = UserExtensionModel.find_by_user_id(rep_id)


Maya it's a good thing to not use abbreviations. It gets really difficult when you're code is worked on by more people. Try not using abbreviations anywhere, could be representative_id.

ok, will change it.

meenakshi-dhanani · 2020-08-12T05:14:52Z

app/api/dao/organization.py

+                }
+        except AttributeError as e:
+            logging.fatal(f"{e}")
+        return messages.IS_ORGANIZATION_REP_NOT_DECLARED, HTTPStatus.FORBIDDEN


what does it mean? IS_ORGANIZATION_REP_NOT_DECLARED? can we name this better?

https://www.codingblocks.net/podcast/clean-code-writing-meaningful-names/ - whenever you're free, Uncle Bob's clean code posts/podcasts are available. They should explain how important it is to have meaningful names

what does it mean? IS_ORGANIZATION_REP_NOT_DECLARED? can we name this better?

This means the user either has not declared whether or not they are an organization representative or has not yet created an additional_info and declare they are a representative. Tbh I can't think of any other name that is more suitable to be the title for this message. Do you have a suggestion @meenakshi-dhanani ?

NO_REPRESENTATIVE_FOR_ORGANIZATION?

Ok, will change it.

ramitsawhney27 · 2020-08-12T07:41:37Z

app/api/dao/organization.py

+        representative_additional_info = UserExtensionModel.find_by_user_id(representative_id)
+        try:
+            if representative_additional_info.is_organization_rep:
+                result = OrganizationModel.find_by_representative(representative_id)


Can this be EAFP/try-catch based? Makes more sense to me intuitively

ok, will try and change it now.

@ramitsawhney27 , I'm not sure how to use EAFP to check if the value of is_organization_rep is true, which is my intention here. Can you please advise?

I meant for line 30-44. We can do:

try:
return {}
except ...:
return messages. OR...

Not a compulsory thing, if you're unclear or disagree, we can skip this for now

I see. Done. I've changed it to EAFP now 😁

ramitsawhney27 · 2020-08-12T07:42:38Z

app/api/dao/organization.py

+            website = data["website"]
+            timezone_value = data["timezone"]
+            timezone = Timezone(timezone_value).name  
+        except KeyError as e:


+1 for try-except

mtreacy002 · 2020-08-12T12:01:27Z

Update @ramitsawhney27 and @anitab-org/bridgeintech-maintainers. I've pushed a minor change to GET /organization so that it returns result if first try is successful. This way it's less code as well (remove unnecessary check on user_id).

mtreacy002 · 2020-08-12T13:30:23Z

Update @anitab-org/bridgeintech-maintainers . I need to refactor Organization dao get response model so that it returns representative name as well as ID to make it meaningful, in particular later since this will be used for viewing Organizations list on UI. So, please do not merge this PR for now.

mtreacy002 · 2020-08-12T14:29:43Z

Update @ramitsawhney27 and @anitab-org/bridgeintech-maintainers .I've pushed the latest change which refactored OrganizationDAO get organization response model to include representative name. I've also modified the tests accordingly. Please re-review when you have time.

meenakshi-dhanani · 2020-08-13T07:16:14Z

Another thing, so I'm doing the GET now and it says-Internal Server Error when I hit get,

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
  "http://www.w3.org/TR/html4/loose.dtd">
<html>
  <head>
    <title>NameError: name 'logging' is not defined // Werkzeug Debugger</title>
    <link rel="stylesheet" href="?__debugger__=yes&amp;cmd=resource&amp;f=style.css"
        type="text/css">
    <!-- We need to make sure this has a favicon so that the debugger does
         not by accident trigger a request to /favicon.ico which might
         change the application state. -->
    <link rel="shortcut icon"
        href="?__debugger__=yes&amp;cmd=resource&amp;f=console.png">
    <script src="?__debugger__=yes&amp;cmd=resource&amp;f=jquery.js"></script>
    <script src="?__debugger__=yes&amp;cmd=resource&amp;f=debugger.js"></script>
    <script type="text/javascript">
      var TRACEBACK = 4500959440,
          CONSOLE_MODE = false,
          EVALEX = true,
          EVALEX_TRUSTED = false,
          SECRET = "hOCDEGO6cGIknxA0j847";
    </script>
  </head>
  <body style="background-color: #fff">
    <div class="debugger">
<h1>NameError</h1>
<div class="detail">

mtreacy002 · 2020-08-13T07:17:26Z

There's also another problem, I updated the additional info to make representative true. And it breaks with
TypeError: Failed to fetch
I guess because I didn't do the GET? Either way this should be handled gracefully ?
GET and PUT are on the same branch, so you can test it on this branch
Can you check on the PUT call, for a user who has represented marked as true, without making the GET call if he/she does the PUT call, what gets returned? For me it gives type error, if it's the same for you can you handle it gracefully maybe 403 the same way you're doing for the other case?

@meenakshi-dhanani . I don't get the same error as you did. I've got the expected 403 GET /organization not sent.

meenakshi-dhanani · 2020-08-13T07:21:15Z

There's also another problem, I updated the additional info to make representative true. And it breaks with
TypeError: Failed to fetch
I guess because I didn't do the GET? Either way this should be handled gracefully ?
GET and PUT are on the same branch, so you can test it on this branch
Can you check on the PUT call, for a user who has represented marked as true, without making the GET call if he/she does the PUT call, what gets returned? For me it gives type error, if it's the same for you can you handle it gracefully maybe 403 the same way you're doing for the other case?
@meenakshi-dhanani . I don't get the same error as you did. I've got the expected 403 GET /organization not sent.

It's because I gave OrganizationStatus as single. It breaks.

meenakshi-dhanani · 2020-08-13T07:22:28Z

There's also another problem, I updated the additional info to make representative true. And it breaks with
TypeError: Failed to fetch
I guess because I didn't do the GET? Either way this should be handled gracefully ?
GET and PUT are on the same branch, so you can test it on this branch
Can you check on the PUT call, for a user who has represented marked as true, without making the GET call if he/she does the PUT call, what gets returned? For me it gives type error, if it's the same for you can you handle it gracefully maybe 403 the same way you're doing for the other case?
@meenakshi-dhanani . I don't get the same error as you did. I've got the expected 403 GET /organization not sent.
It's because I gave OrganizationStatus as single. It breaks.

and the timezone too. The code breaks, can we handle these as BAD REQUEST?

mtreacy002 · 2020-08-13T07:23:58Z

Another thing, so I'm doing the GET now and it says-Internal Server Error when I hit get,

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
  "http://www.w3.org/TR/html4/loose.dtd">
<html>
  <head>
    <title>NameError: name 'logging' is not defined // Werkzeug Debugger</title>
    <link rel="stylesheet" href="?__debugger__=yes&amp;cmd=resource&amp;f=style.css"
        type="text/css">
    <!-- We need to make sure this has a favicon so that the debugger does
         not by accident trigger a request to /favicon.ico which might
         change the application state. -->
    <link rel="shortcut icon"
        href="?__debugger__=yes&amp;cmd=resource&amp;f=console.png">
    <script src="?__debugger__=yes&amp;cmd=resource&amp;f=jquery.js"></script>
    <script src="?__debugger__=yes&amp;cmd=resource&amp;f=debugger.js"></script>
    <script type="text/javascript">
      var TRACEBACK = 4500959440,
          CONSOLE_MODE = false,
          EVALEX = true,
          EVALEX_TRUSTED = false,
          SECRET = "hOCDEGO6cGIknxA0j847";
    </script>
  </head>
  <body style="background-color: #fff">
    <div class="debugger">
<h1>NameError</h1>
<div class="detail">

Mmmm.... have you tried to pull my latest code? I did make the push a couple of times. The reason I asked is because I didn't get the same error for PUT.

mtreacy002 · 2020-08-13T07:32:22Z

There's also another problem, I updated the additional info to make representative true. And it breaks with
TypeError: Failed to fetch
I guess because I didn't do the GET? Either way this should be handled gracefully ?
GET and PUT are on the same branch, so you can test it on this branch
Can you check on the PUT call, for a user who has represented marked as true, without making the GET call if he/she does the PUT call, what gets returned? For me it gives type error, if it's the same for you can you handle it gracefully maybe 403 the same way you're doing for the other case?
@meenakshi-dhanani . I don't get the same error as you did. I've got the expected 403 GET /organization not sent.
It's because I gave OrganizationStatus as single. It breaks.
and the timezone too. The code breaks, can we handle these as BAD REQUEST?

Ok, will fix the error catching of Timezone and Status enum too

mtreacy002 · 2020-08-13T14:39:00Z

Update @meenakshi-dhanani and @anitab-org/bridgeintech-maintainers . I've just pushed the latest change that addresses Meena's feedback on error handling and setting user cookie upon login.
Note that the changes I've made here only to the main code, not test cases (which is why Travis is failing). I'm gonna work on refactoring the test cases now. It's a major change btw, so bare with me. I'll take a while.

mtreacy002 · 2020-08-14T10:42:21Z

Update @meenakshi-dhanani and @anitab-org/bridgeintech-maintainers . I've refactored all test cases to align with the main code changes. This PR is now completed. Can you please check it out when you have time? Thanks

meenakshi-dhanani · 2020-08-14T19:21:23Z

It's still breaking for invalid status, can you check?

mtreacy002 · 2020-08-15T00:44:21Z

@meenakshi-dhanani . Did you pull on my last change <commit 983c053>? Also can you show me screenshot of what's breaking? It's not clear which invalid status you're talking about.
Invalid token, invalid input fields, or invalid data validations?

Added create and update functionalities Add tests for get, create and update organization Fix rep_id abbreviation Use EAFP to check if organization exist Fix bug UnboundlocalError if user_id remove unnecessary reeturn line Refactor get organization response model and tests fix rep_id and response message Refactor rep_ syntax, cookie user retrieval on login, error handling 400 for invalid input fields Refactor test cases Fix update organization test cases Remove the note on API details on the need to do GET/user/personal_details first Fix invalid status response

mtreacy002 · 2020-08-15T01:09:48Z

@meenakshi-dhanani and @anitab-org/bridgeintech-maintainers . I've fixed the invalid status response and pushed the change. Can you check?

meenakshi-dhanani · 2020-08-15T12:23:36Z

Looks good. I checked the status, email, timezone and validations. Approving

meenakshi-dhanani · 2020-08-15T12:24:30Z

@ramitsawhney27 you would need to re-approve.

mtreacy002 force-pushed the post-get-organization-and-tests branch from 84e9236 to 753a1a2 Compare August 11, 2020 11:38

mtreacy002 force-pushed the post-get-organization-and-tests branch from 753a1a2 to 147a472 Compare August 12, 2020 04:41

mtreacy002 changed the title ~~WIP | Feat: PUT and GET organization and tests~~ Feat: PUT and GET organization and tests Aug 12, 2020

mtreacy002 requested review from ramitsawhney27 and a team August 12, 2020 04:58

mtreacy002 self-assigned this Aug 12, 2020

mtreacy002 added Category: Coding Changes to code base or refactored code that doesn't fix a bug. Program: GSOC Related to work completed during the Google Summer of Code Program. labels Aug 12, 2020

meenakshi-dhanani reviewed Aug 12, 2020

View reviewed changes

mtreacy002 force-pushed the post-get-organization-and-tests branch from 147a472 to 8a22fcf Compare August 12, 2020 07:09

ramitsawhney27 reviewed Aug 12, 2020

View reviewed changes

mtreacy002 force-pushed the post-get-organization-and-tests branch from 8a22fcf to 81fc1bb Compare August 12, 2020 10:23

ramitsawhney27 previously approved these changes Aug 12, 2020

View reviewed changes

mtreacy002 dismissed ramitsawhney27’s stale review via dff7193 August 12, 2020 11:58

mtreacy002 force-pushed the post-get-organization-and-tests branch from 81fc1bb to dff7193 Compare August 12, 2020 11:58

mtreacy002 force-pushed the post-get-organization-and-tests branch from dff7193 to 9f72020 Compare August 12, 2020 12:07

mtreacy002 changed the title ~~Feat: PUT and GET organization and tests~~ Feat: PUT and GET organization and tests [DO NOT MERGE] Aug 12, 2020

mtreacy002 added the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Aug 12, 2020

mtreacy002 force-pushed the post-get-organization-and-tests branch from 9f72020 to 95f2745 Compare August 12, 2020 14:27

mtreacy002 changed the title ~~Feat: PUT and GET organization and tests [DO NOT MERGE]~~ Feat: PUT and GET organization and tests Aug 12, 2020

mtreacy002 force-pushed the post-get-organization-and-tests branch from 59d0489 to 22207a8 Compare August 13, 2020 14:34

mtreacy002 mentioned this pull request Aug 13, 2020

Feat: GET organizations list #107

Merged

8 tasks

mtreacy002 force-pushed the post-get-organization-and-tests branch from 22207a8 to 1b4801f Compare August 14, 2020 10:40

mtreacy002 requested review from meenakshi-dhanani and ramitsawhney27 August 14, 2020 10:44

mtreacy002 force-pushed the post-get-organization-and-tests branch 3 times, most recently from e892166 to 983c053 Compare August 14, 2020 13:13

mtreacy002 force-pushed the post-get-organization-and-tests branch from 983c053 to 2dc6166 Compare August 15, 2020 01:07

meenakshi-dhanani approved these changes Aug 15, 2020

View reviewed changes

ramitsawhney27 approved these changes Aug 15, 2020

View reviewed changes

ramitsawhney27 merged commit 3b85fe2 into anitab-org:develop Aug 15, 2020

mtreacy002 mentioned this pull request Aug 24, 2020

Feat view user programs list and program anitab-org/bridge-in-tech-web#76

Closed

5 tasks

mtreacy002 deleted the post-get-organization-and-tests branch August 30, 2020 11:29

anishghimire603 mentioned this pull request Sep 19, 2020

Doc: Created GETTING_STARTED.md file inside .github folder #144

Merged

3 tasks

Feat: PUT and GET organization and tests #103

Feat: PUT and GET organization and tests #103

Conversation

mtreacy002 commented Aug 10, 2020 • edited Loading

Description

Type of Change:

How Has This Been Tested?

Checklist:

mtreacy002 commented Aug 10, 2020

mtreacy002 commented Aug 10, 2020

mtreacy002 commented Aug 11, 2020 • edited Loading

mtreacy002 commented Aug 11, 2020

ramitsawhney27 commented Aug 11, 2020

mtreacy002 commented Aug 12, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 commented Aug 12, 2020 • edited Loading

mtreacy002 commented Aug 12, 2020

mtreacy002 commented Aug 12, 2020

meenakshi-dhanani commented Aug 13, 2020

mtreacy002 commented Aug 13, 2020

meenakshi-dhanani commented Aug 13, 2020

meenakshi-dhanani commented Aug 13, 2020

mtreacy002 commented Aug 13, 2020

mtreacy002 commented Aug 13, 2020

mtreacy002 commented Aug 13, 2020

mtreacy002 commented Aug 14, 2020

meenakshi-dhanani commented Aug 14, 2020

mtreacy002 commented Aug 15, 2020

mtreacy002 commented Aug 15, 2020

meenakshi-dhanani commented Aug 15, 2020

meenakshi-dhanani commented Aug 15, 2020

mtreacy002 commented Aug 10, 2020 •

edited

Loading

mtreacy002 commented Aug 11, 2020 •

edited

Loading

mtreacy002 commented Aug 12, 2020 •

edited

Loading