Feat: View/Create and Update Organization Profile #69

mtreacy002 · 2020-08-17T05:57:47Z

Description

Added View Organization Profile Functionality

Fixes #50, #51

Type of Change:

Code
User Interface

Code/Quality Assurance Only

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

How Has This Been Tested?

login as user who has an additional info with is_organization_rep value True but has not created an organization.

on View organization profile

go to MyOrganization > Profile. On successful GET /organization see response below

On Create organization profile

Add details for the organization and click Save. On successful create, see response below.

To check if the data is saved to database, move away from the MyOrganization Profile page (click on any other page) and comeback. See response below:

On Update organization profile:

change the details on the organization page and click Save. On successful update, see response below:

to check if the data is updated, move away from thee MyOrganization Profile page (click on any other page) and comeback. See response below:

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

Code/Quality Assurance Only

My changes generate no new warnings
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
Any dependent changes have been published in downstream modules

Additional note:
This PR is to be tested with the Backend server from PR#112 only since it contains the refactored login with successful response returning is_organization_representative value.

mtreacy002 · 2020-08-17T06:07:54Z

Update @anitab-org/bridgeintech-maintainers . I've completed View/Create/Update Organization profile on this one PR. Since we only have one week to go to complete the project features, I'll move on to the next functionalities first which are the View organizations and view other organization profile (where user has selected the organization name from the Organizations list).
@isabelcosta and @meenakshi-dhanani , is it ok if for now I do the remaining testing on UI (starting from View Additional Info onwards) after I complete the remaining deliverables (still have Programs and Send application Form to a Mentoring Program backend and frontend after Organizations UI completed). If all goes well, this week I'll complete all deliverables, theen I can finish off the testing UI next week whilee wrapping up. Let me know what you think.

mtreacy002 · 2020-08-18T00:46:26Z

@meenakshi-dhanani and @ramitsawhney27 , I want to point out that atm I didn't show the join date info of the organization. This is because I'm not sure where to put the logic of converting the date to human readable format. If we follow the enum logic which is kept at backend, I need to refactor backend to add that logic (converting date for GET and PUT API endpoints). However, I remembered that we had that discussion on the initial stage of development that dates format on backend is to be kept in the UNIX as to keep it as close to the machine language. Can you both please share your opinion on this, whether to put the conversion logic on the frontend or backend? Thanks.

meenakshi-dhanani · 2020-08-18T18:57:48Z

@meenakshi-dhanani and @ramitsawhney27 , I want to point out that atm I didn't show the join date info of the organization. This is because I'm not sure where to put the logic of converting the date to human readable format. If we follow the enum logic which is kept at backend, I need to refactor backend to add that logic (converting date for GET and PUT API endpoints). However, I remembered that we had that discussion on the initial stage of development that dates format on backend is to be kept in the UNIX as to keep it as close to the machine language. Can you both please share your opinion on this, whether to put the conversion logic on the frontend or backend? Thanks.

Getting back to the earlier discussion, it wasn't just about keeping the detail in the backend. One thing we agreed on was keeping it in one place. Since all of the timezone information was all on the backend, we chose to not do a larger change and thus stuck to having it in the backend. If it's not your core business logic though, and if it's presentation logic feel free to do things on the frontend.

In this case I don't have a strong reason for why the calculation needs to be in the frontend, though I usually have seen APIs returning readable dates from the backend, eg. 2020-01-06T20:22:05.000Z. While using the API, just as an API, it's far more convenient to have in human readable format. I'd prefer backend.

mtreacy002 · 2020-08-19T11:34:28Z

@meenakshi-dhanani and @ramitsawhney27 , I want to point out that atm I didn't show the join date info of the organization. This is because I'm not sure where to put the logic of converting the date to human readable format. If we follow the enum logic which is kept at backend, I need to refactor backend to add that logic (converting date for GET and PUT API endpoints). However, I remembered that we had that discussion on the initial stage of development that dates format on backend is to be kept in the UNIX as to keep it as close to the machine language. Can you both please share your opinion on this, whether to put the conversion logic on the frontend or backend? Thanks.

Getting back to the earlier discussion, it wasn't just about keeping the detail in the backend. One thing we agreed on was keeping it in one place. Since all of the timezone information was all on the backend, we chose to not do a larger change and thus stuck to having it in the backend. If it's not your core business logic though, and if it's presentation logic feel free to do things on the frontend.

In this case I don't have a strong reason for why the calculation needs to be in the frontend, though I usually have seen APIs returning readable dates from the backend, eg. 2020-01-06T20:22:05.000Z. While using the API, just as an API, it's far more convenient to have in human readable format. I'd prefer backend.

Thanks, @meenakshi-dhanani for sharing your thoughts. @ramitsawhney27, can I also please have your thoughts on this? If we decide to do the logic at the backend, before moving to creating View and Update Programs on frontend, I'll refactor the relevant date related fields on any endpoints that are currently using the unix dates to accept the human readable format (perhaps RFC 2822: Thursday, 31-Dec-20 00:00:00 UTC). But the database will be kept in double precision format.

meenakshi-dhanani · 2020-08-22T14:05:33Z

We use context when there is some global state that we want a lot of components to use, eg. login, logout, etc.

If a user is a representative, is this state required across the application?
Also, a cookie has data that will disappear if the client clears it. This gives a wrong picture. And cookie data can be modified to get access to my organization pages. There can also be cases, when the the backend says something and the cookie reflects differently.

In case a user is a representative, should one call suffice? Is it really necessary to keep it in context?
And definitely this information should not be part of a cookie

meenakshi-dhanani · 2020-08-22T14:10:08Z

What we can do is shift the My Organization as a separate submenu inside My Space. We can simply show that irrespective of whether a user is a representative or no, if a user is not representative, let that page not show up. If the user is a representative then let's show the form.
Also, let's get the information whether a user is representative or no from an api call. I see the usage of is_representative in this component only so let's not keep it as part of the AuthContext also because it has no relation to authentication

mtreacy002 · 2020-08-22T14:21:03Z

 If a user is a representative, is this state required across the application?

@meenakshi-dhanani . is_organization_representative in the frontend is used to set thee Navbar which is re-rendered on each pages user go to. So, in this case, I would say, yes, the value needs to be placed in Context.

In case a user is a representative, should one call suffice? Is it really necessary to keep it in context?

as per the answer above, it's needed on the Navbar

And definitely this information should not be part of a cookie

Yeah, I think keeping it in AuthContext will suffice. Do you agree?

mtreacy002 · 2020-08-22T14:24:51Z

What we can do is shift the My Organization as a separate submenu inside My Space. We can simply show that irrespective of whether a user is a representative or no, if a user is not representative, let that page not show up. If the user is a representative then let's show the form.
Also, let's get the information whether a user is representative or no from an api call. I see the usage of is_representative in this component only so let's not keep it as part of the AuthContext also because it has no relation to authentication

On this point, @meenakshi-dhanani , since the Navbar menu gets re-rendered every time user go to different pages, the submenu My Organisation will be checked against the is_organization_rep every time a new page is rendered. If not in AuthContext, how would you suggest we keep the value of this attribute?

meenakshi-dhanani · 2020-08-22T14:55:03Z

What we can do is shift the My Organization as a separate submenu inside My Space. We can simply show that irrespective of whether a user is a representative or no, if a user is not representative, let that page not show up. If the user is a representative then let's show the form.
Also, let's get the information whether a user is representative or no from an api call. I see the usage of is_representative in this component only so let's not keep it as part of the AuthContext also because it has no relation to authentication

On this point, @meenakshi-dhanani , since the Navbar menu gets re-rendered every time user go to different pages, the submenu My Organisation will be checked against the is_organization_rep every time a new page is rendered. If not in AuthContext, how would you suggest we keep the value of this attribute?

What I'm suggesting is this need not be in the navbar for now. You can keep my organization inside my space. Similar to how you handle personal information. If a user doesn't have any information you say that the user is not a representative, if the user wants to be then the user must check the box in additional details. If the user is then show the form. Also, the fact that a user is representative can be received via an API call that could be made.

The responsibility of the AuthContext is not to keep other fields, it's just the access token that is very tightly coupled with authentication. So definitely not inside AuthContext.

You could do an api call that includes in it's response - is_representative details
Maybe the additional-details call returns that flag right? make that call and fetch from there?

Also the responsibility and expected payload of a login API is not to return the is_representative_flag. So in both ways it's not right

mtreacy002 · 2020-08-22T14:59:48Z

What we can do is shift the My Organization as a separate submenu inside My Space. We can simply show that irrespective of whether a user is a representative or no, if a user is not representative, let that page not show up. If the user is a representative then let's show the form.
Also, let's get the information whether a user is representative or no from an api call. I see the usage of is_representative in this component only so let's not keep it as part of the AuthContext also because it has no relation to authentication

On this point, @meenakshi-dhanani , since the Navbar menu gets re-rendered every time user go to different pages, the submenu My Organisation will be checked against the is_organization_rep every time a new page is rendered. If not in AuthContext, how would you suggest we keep the value of this attribute?

What I'm suggesting is this need not be in the navbar for now. You can keep my organization inside my space. Similar to how you handle personal information. If a user doesn't have any information you say that the user is not a representative, if the user wants to be then the user must check the box in additional details. If the user is then show the form. Also, the fact that a user is representative can be received via an API call that could be made.

The responsibility of the AuthContext is not to keep other fields, it's just the access token that is very tightly coupled with authentication. So definitely not inside AuthContext.

You could do an api call that includes in it's response - is_representative details
Maybe the additional-details call returns that flag right? make that call and fetch from there?

Also the responsibility and expected payload of a login API is not to return the is_representative_flag. So in both ways it's not right

Ok, Can you please open a PR for the refactoring? both on backend and frontend? I'll do it post GSoC.

mtreacy002 · 2020-08-23T05:00:38Z

@meenakshi-dhanani Thanks for your help sorting out the Navbar. I've merged the PR you raised to this one. Can you please re-review? Thanks. cc @anitab-org/bridgeintech-maintainers

meenakshi-dhanani · 2020-08-23T05:19:17Z

src/AuthContext.js

@@ -36,6 +37,7 @@ function AuthProvider({ children }) {
    Cookies.remove("user");
    Cookies.remove("access_token");
    Cookies.remove("access_expiry");
+    Cookies.remove("is_organization_representative");


this line should not be there

Done. It's now removed

meenakshi-dhanani · 2020-08-23T05:25:54Z

@meenakshi-dhanani Thanks for your help sorting out the Navbar. I've merged the PR you raised to this one. Can you please re-review? Thanks. cc @anitab-org/bridgeintech-maintainers

We just need to remove that line in AuthContext, and I will approve

mtreacy002 · 2020-08-23T05:27:08Z

@meenakshi-dhanani Thanks for your help sorting out the Navbar. I've merged the PR you raised to this one. Can you please re-review? Thanks. cc @anitab-org/bridgeintech-maintainers

We just need to remove that line in AuthContext, and I will approve

Done that, @meenakshi-dhanani .

meenakshi-dhanani · 2020-08-23T06:17:16Z

meenakshi-dhanani · 2020-08-23T07:55:43Z

@ramitsawhney27 all changes LGTM, can you approve if it looks fine?

meenakshi-dhanani · 2020-08-23T15:06:44Z

@isabelcosta I have attached the .gif above that shows the following:

Displays organization not declared for a user, who is not an organization representative
Edit organization for a user that represents an organization
Can you check?

meenakshi-dhanani

I took the latest pull and ran again, I had missed the following scenario:

If a organization does not exist for a user, allow the user to create the organization when Organization profile is clicked under my organization. This is breaking, needs to be fixed.

isabelcosta · 2020-08-23T22:31:09Z

@meenakshi-dhanani can you explain the bug a little bit more, I didn't understand your sentence 😕

remove is_representative from cookie and fix navigation Remove is_organization_rep fix Create Organization

mtreacy002 · 2020-08-23T23:32:46Z

@meenakshi-dhanani , I've fixed the initial page rendering so user can fill in the form to create organization if they haven't create one (just as what I had before the Navbar fix as can be seen in this PR description)

meenakshi-dhanani · 2020-08-24T07:38:01Z

@isabelcosta The bug I reported, was about a user, that says that he/she/they are representing an organization, but have no organization created. The expected behavior would be to see a create organization form for them. Maya has pushed the fix for that now. So that is working with the fix.

I have tested the following scenarios:

Displays organization not declared for a user, for a user who is not an organization representative. Still shows the form though, we could think of the UX in another issue.
Edit organization for a user that represents an organization and has an organization
Add an organization for a user that represents an org but has no org created

foongminwong

The changes made in this PR were tested locally. Following are the results:

Code review - Done
All possible responses were tested as below:
- View/Create and Update organization Profile
  Screenshot/gif/url:
Expected Result: As a user, i should be able to view, create, update org profile as an org rep.
Actual Result: Same as expected
OS Version: Windows 10

mtreacy002 self-assigned this Aug 17, 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 17, 2020

mtreacy002 mentioned this pull request Aug 18, 2020

Feat view organizations list and other organization profile #70

Merged

5 tasks

mtreacy002 mentioned this pull request Aug 22, 2020

Refactor login to return is organization representative anitab-org/bridge-in-tech-backend#112

Closed

8 tasks

mtreacy002 requested review from meenakshi-dhanani and a team August 23, 2020 05:00

meenakshi-dhanani reviewed Aug 23, 2020

View reviewed changes

mtreacy002 requested review from meenakshi-dhanani and a team August 23, 2020 05:27

meenakshi-dhanani previously approved these changes Aug 23, 2020

View reviewed changes

mtreacy002 dismissed meenakshi-dhanani’s stale review via 742ce0b August 23, 2020 06:15

mtreacy002 force-pushed the Feat--View-Organization-profile branch from 6ad5a14 to 742ce0b Compare August 23, 2020 06:15

meenakshi-dhanani approved these changes Aug 23, 2020

View reviewed changes

meenakshi-dhanani requested a review from isabelcosta August 23, 2020 15:06

meenakshi-dhanani suggested changes Aug 23, 2020

View reviewed changes

Feat: View Organization Profile

b7d74ca

remove is_representative from cookie and fix navigation Remove is_organization_rep fix Create Organization

mtreacy002 force-pushed the Feat--View-Organization-profile branch from 742ce0b to b7d74ca Compare August 23, 2020 23:31

meenakshi-dhanani approved these changes Aug 24, 2020

View reviewed changes

foongminwong approved these changes Aug 25, 2020

View reviewed changes

meenakshi-dhanani merged commit 2cb9b3d into anitab-org:develop Aug 25, 2020

mtreacy002 mentioned this pull request Aug 27, 2020

Create or Update Organization Profile #51

Closed

3 tasks

mtreacy002 deleted the Feat--View-Organization-profile branch September 12, 2020 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: View/Create and Update Organization Profile #69

Feat: View/Create and Update Organization Profile #69

mtreacy002 commented Aug 17, 2020 •

edited

Loading

mtreacy002 commented Aug 17, 2020

mtreacy002 commented Aug 18, 2020

meenakshi-dhanani commented Aug 18, 2020

mtreacy002 commented Aug 19, 2020

meenakshi-dhanani commented Aug 22, 2020

meenakshi-dhanani commented Aug 22, 2020

mtreacy002 commented Aug 22, 2020

mtreacy002 commented Aug 22, 2020 •

edited

Loading

meenakshi-dhanani commented Aug 22, 2020 •

edited

Loading

mtreacy002 commented Aug 22, 2020

mtreacy002 commented Aug 23, 2020

meenakshi-dhanani Aug 23, 2020

mtreacy002 Aug 23, 2020

meenakshi-dhanani commented Aug 23, 2020

mtreacy002 commented Aug 23, 2020

meenakshi-dhanani commented Aug 23, 2020

meenakshi-dhanani commented Aug 23, 2020

meenakshi-dhanani commented Aug 23, 2020

meenakshi-dhanani left a comment

isabelcosta commented Aug 23, 2020

mtreacy002 commented Aug 23, 2020 •

edited

Loading

meenakshi-dhanani commented Aug 24, 2020 •

edited

Loading

foongminwong left a comment

Feat: View/Create and Update Organization Profile #69

Feat: View/Create and Update Organization Profile #69

Conversation

mtreacy002 commented Aug 17, 2020 • edited Loading

Description

Type of Change:

How Has This Been Tested?

Checklist:

mtreacy002 commented Aug 17, 2020

mtreacy002 commented Aug 18, 2020

meenakshi-dhanani commented Aug 18, 2020

mtreacy002 commented Aug 19, 2020

meenakshi-dhanani commented Aug 22, 2020

meenakshi-dhanani commented Aug 22, 2020

mtreacy002 commented Aug 22, 2020

mtreacy002 commented Aug 22, 2020 • edited Loading

meenakshi-dhanani commented Aug 22, 2020 • edited Loading

mtreacy002 commented Aug 22, 2020

mtreacy002 commented Aug 23, 2020

meenakshi-dhanani Aug 23, 2020

Choose a reason for hiding this comment

mtreacy002 Aug 23, 2020

Choose a reason for hiding this comment

meenakshi-dhanani commented Aug 23, 2020

mtreacy002 commented Aug 23, 2020

meenakshi-dhanani commented Aug 23, 2020

meenakshi-dhanani commented Aug 23, 2020

meenakshi-dhanani commented Aug 23, 2020

meenakshi-dhanani left a comment

Choose a reason for hiding this comment

isabelcosta commented Aug 23, 2020

mtreacy002 commented Aug 23, 2020 • edited Loading

meenakshi-dhanani commented Aug 24, 2020 • edited Loading

foongminwong left a comment

Choose a reason for hiding this comment

mtreacy002 commented Aug 17, 2020 •

edited

Loading

mtreacy002 commented Aug 22, 2020 •

edited

Loading

meenakshi-dhanani commented Aug 22, 2020 •

edited

Loading

mtreacy002 commented Aug 23, 2020 •

edited

Loading

meenakshi-dhanani commented Aug 24, 2020 •

edited

Loading