Feat: Issue#35 view user additional information #40

mtreacy002 · 2020-07-17T05:59:48Z

Description

Allow user to view their additional infomation page

Fixes #35

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?

By login with as a user and geet additional information page by following the path:
My Space -> Profile -> on Personal Details page hit Continue button and see below screenshots:

if user is a representative of an organization:
if user is not a repreesentative of an organization:

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials

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)
New and existing unit tests pass locally with my changes

Additional Note
This is a WIP. Still need to add tests and fix bugs

mtreacy002 · 2020-07-17T06:03:37Z

Update @meenakshi-dhanani and @anitab-org/bridgeintech-maintainers . Here's the PR for View User's Additional Information page. This is still WIP as it doesn't have tests yet and still have minor bugs need to be fixed (to do with routing/navigating to previous page)

meenakshi-dhanani · 2020-07-19T11:45:00Z

src/Enum.js

@@ -0,0 +1,29 @@
+export const TIMEZONE_ENUM = {


there is no enum in javascript. There are objects that have key value pairs. You can simply call it timezones

@meenakshi-dhanani , the reason I did this is because the backend respond to this request with the timezone enum name

which is what's being recorded in postgresql.

However, what I want the user to see is the timezone value which according to this stackoverflow discussion could be done in in this way (what I've done in the code).

The difference of the code are as per screenshoot below (the one commented out is returning timezone as its name, not value)

There is another alternative, which is to change the backend response and request model to accept/return human-readable format (aka enum value) from the backend side and do the logic to convert it to enum name before saving it to the database.
But again, seeing how we keep machine format (unix datetime) for datetime on our backend and only change it to human-readable on frontend, I thought there's a convention somewhere (I'm not sure if there is, but @ramitsawhney27 maybe can clarify this for us) to keep backend as close as machine language

Choose to keep the mapping logic in the frontend/backend. And follow it consistently. Based on our discussion, keeping it in the backend right now is less effort. So you can continue with that. So frontend will not keep any mappings, as per my understanding

Ok. will change it now.

meenakshi-dhanani · 2020-07-19T13:32:00Z

src/myspace/AdditionalInfo.jsx

+  useEffect(() => {
+    fetch(`${BASE_API}/user/additional_info`, requestAdditionalInfo)
+      .then(async response => {
+


Why the line?

got it. now removed.

meenakshi-dhanani · 2020-07-19T13:32:13Z

src/myspace/AdditionalInfo.jsx

+        setAdditionalInfo(data);
+      })
+      .catch(error => {
+        setErrorMessage(data["message"]);


can you make this data.message?

src/myspace/AdditionalInfo.jsx

mtreacy002 · 2020-07-20T03:12:31Z

update @meenakshi-dhanani and @anitab-org/bridgeintech-maintainers . I've pushed the latest chage. Please re-review.

src/myspace/AdditionalInfo.jsx

meenakshi-dhanani · 2020-07-20T15:58:44Z

src/myspace/AdditionalInfo.jsx

+    fetch(`${BASE_API}/user/additional_info`, requestAdditionalInfo)
+      .then(async response => {
+        data = await response.json();
+        data.message ? setErrorMessage(data.message) : setAdditionalInfo(data);


error message is already getting set in catch right? would there be a case that on error the control would not go to a catch? Just wondering?

@meenakshi-dhanani . For some reason, it seems like only the uncustomised errors get inside catch. where as the customised errors did not get inside catch. you can test this by running the app, login, but skip going to personal details page and go straight to additional info. with error also set inside get, you'll see the error message. but if you comment it out and just relying from the one inside catch, you'll not see the error message. I don't know why this happen, but maybe just the way fetch is, or perhaps something in backend need to be change. Any suggestions? cc @ramitsawhney27

@mtreacy002 I checked fetch it works that way. Doesn't reject a 400. I'm looking at why and figuring another way. I'll post links on this soon.

I checked the documentation - https://javascript.info/fetch. Fetch doesn't reject http status errors. Usually how we handle it is since this same logic will be repeated for every call, we have a wrapper over api calls. You can push this and I can open another PR for that. However even in this, what you'd want to do it, check if the !response.ok and reject the promise with the data. Let catch then be the only place where you setErrorMessage. Once you reject anything, it will go to the catch

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/reject - Inside the new error you can give the data from response.json

got it. made the change. will push now

mtreacy002 · 2020-07-21T02:42:08Z

Update @anitab-org/bridgeintech-maintainers . I've added backend Heroku pr app server url to config. I may do the same on the next PR as well (not promising) so you can test frontend with the matching PR Heroku backend server instead of running locally

mtreacy002 · 2020-07-22T05:09:52Z

Update @meenakshi-dhanani and @anitab-org/bridgeintech-maintainers . I've made the requested changes. Can you please re-review? Ta

meenakshi-dhanani · 2020-07-22T11:07:54Z

src/myspace/AdditionalInfo.jsx

+import { BASE_API } from "../config";
+import { AuthContext } from "../AuthContext";
+import "./MySpace.css";
+import { set } from "js-cookie";


this is unused

forgot this one too. Now it''s removed

meenakshi-dhanani · 2020-07-22T11:08:44Z

src/myspace/AdditionalInfo.jsx

+    },
+  };
+
+  const status = (response) => {


this const status is also not being used. You can remove this

oops.. forgot to remove it. Ok. done now. 😉

src/myspace/AdditionalInfo.jsx

Remove buttons from the form and fix navigation Replace cookies with token from authcontext Refactor timezone logic and fix data setting Add Heroku PR app url to config for backend server alternative Throw error if response not 200 throw error if response not 200 for personal details remove unused constant remove unused import Remove extra closing curly bracket Refactored throw to Error(data)

meenakshi-dhanani · 2020-07-24T11:26:04Z

src/myspace/AdditionalInfo.jsx

+        if (response.ok) {
+          setAdditionalInfo(data);
+        } else {
+          throw new Error(data);


we can look at this today in our call - https://www.geeksforgeeks.org/reject-vs-throw-promises-in-javascript/

meenakshi-dhanani · 2020-07-24T14:15:15Z

@ramitsawhney27 @foongminwong can you approve if it looks fine? I reviewed it with Maya

foongminwong

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

Code review - Done
All possible responses were tested as below:

PR 40
Screenshot/gif/url:

Expected Result: As a user, I should be able to view user additional information. If user additional information is not present, it should notify the user to click o the Personal Details page first.
Actual Result: Same as expected.
OS Version: Windows 10

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

mtreacy002 added this to the GSoC: Coding Phase II milestone Jul 17, 2020

mtreacy002 changed the title ~~WIP | Feat: Issue#35 view user additional information~~ Feat: Issue#35 view user additional information Jul 17, 2020

mtreacy002 force-pushed the issue#35-view-user-additional-information branch 2 times, most recently from a2c0571 to 15baa59 Compare July 18, 2020 01:11

meenakshi-dhanani reviewed Jul 19, 2020

View reviewed changes

src/myspace/AdditionalInfo.jsx Show resolved Hide resolved

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

mtreacy002 changed the title ~~Feat: Issue#35 view user additional information~~ WIP | Feat: Issue#35 view user additional information Jul 19, 2020

mtreacy002 force-pushed the issue#35-view-user-additional-information branch 2 times, most recently from 352e841 to abd5e13 Compare July 20, 2020 03:09

mtreacy002 requested review from meenakshi-dhanani and a team July 20, 2020 03:12

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

mtreacy002 changed the title ~~WIP | Feat: Issue#35 view user additional information~~ Feat: Issue#35 view user additional information Jul 20, 2020

meenakshi-dhanani reviewed Jul 20, 2020

View reviewed changes

src/myspace/AdditionalInfo.jsx Show resolved Hide resolved

meenakshi-dhanani reviewed Jul 20, 2020

View reviewed changes

mtreacy002 force-pushed the issue#35-view-user-additional-information branch from abd5e13 to e97c918 Compare July 21, 2020 02:39

mtreacy002 force-pushed the issue#35-view-user-additional-information branch 2 times, most recently from ab8b294 to b42e230 Compare July 22, 2020 05:08

mtreacy002 requested review from meenakshi-dhanani and a team July 22, 2020 05:09

meenakshi-dhanani reviewed Jul 22, 2020

View reviewed changes

mtreacy002 force-pushed the issue#35-view-user-additional-information branch 3 times, most recently from 7c2aca8 to 6404f62 Compare July 22, 2020 13:14

meenakshi-dhanani reviewed Jul 22, 2020

View reviewed changes

src/myspace/AdditionalInfo.jsx Show resolved Hide resolved

mtreacy002 force-pushed the issue#35-view-user-additional-information branch from 6404f62 to 6b990f1 Compare July 23, 2020 01:44

meenakshi-dhanani reviewed Jul 24, 2020

View reviewed changes

Display service unavailable message when server is down

46fea84

meenakshi-dhanani approved these changes Jul 24, 2020

View reviewed changes

foongminwong approved these changes Jul 25, 2020

View reviewed changes

foongminwong merged commit 59eac9b into anitab-org:develop Jul 25, 2020

mtreacy002 deleted the issue#35-view-user-additional-information branch September 12, 2020 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Issue#35 view user additional information #40

Feat: Issue#35 view user additional information #40

mtreacy002 commented Jul 17, 2020

mtreacy002 commented Jul 17, 2020

meenakshi-dhanani Jul 19, 2020

mtreacy002 Jul 19, 2020 •

edited

Loading

meenakshi-dhanani Jul 19, 2020

mtreacy002 Jul 20, 2020

meenakshi-dhanani Jul 19, 2020

mtreacy002 Jul 20, 2020

meenakshi-dhanani Jul 19, 2020

mtreacy002 Jul 20, 2020

mtreacy002 commented Jul 20, 2020

meenakshi-dhanani Jul 20, 2020

mtreacy002 Jul 20, 2020 •

edited

Loading

meenakshi-dhanani Jul 21, 2020

meenakshi-dhanani Jul 21, 2020

meenakshi-dhanani Jul 21, 2020

mtreacy002 Jul 22, 2020

mtreacy002 commented Jul 21, 2020

mtreacy002 commented Jul 22, 2020

meenakshi-dhanani Jul 22, 2020

mtreacy002 Jul 22, 2020

meenakshi-dhanani Jul 22, 2020

mtreacy002 Jul 22, 2020

meenakshi-dhanani Jul 24, 2020

meenakshi-dhanani commented Jul 24, 2020

foongminwong left a comment

Feat: Issue#35 view user additional information #40

Feat: Issue#35 view user additional information #40

Conversation

mtreacy002 commented Jul 17, 2020

Description

Type of Change:

How Has This Been Tested?

Checklist:

mtreacy002 commented Jul 17, 2020

Choose a reason for hiding this comment

mtreacy002 Jul 19, 2020 • edited Loading

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 Jul 20, 2020

Choose a reason for hiding this comment

mtreacy002 Jul 20, 2020 • edited Loading

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 Jul 21, 2020

mtreacy002 commented Jul 22, 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

meenakshi-dhanani commented Jul 24, 2020

foongminwong left a comment

Choose a reason for hiding this comment

mtreacy002 Jul 19, 2020 •

edited

Loading

mtreacy002 Jul 20, 2020 •

edited

Loading