[change] Do not send new SMS unless needed #656 #665
Conversation
There was a problem hiding this comment.
Looks good and works as expected during testing!
I have 2 requests.
1. Backward Compatibility
I think that if we try to use this version of OWLP with an older version of OpenWISP Radius, the SMS token verification process will break because the active token view is not available.
Can you please make the code resilient to this failure? Here's how it could work:
- if the nodeJS backend gets 404 from openwisp-radius, it shall forward the 404
- if the frontend JS code gets 404, it assumes it has to send a new verification code (old behavior is kept for backward compatibility).
What do you think?
2. I found a React Warning during testing
Can you see if you get this warning when reloading the phone number verification page?
Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
in Status (created by Connect(Status))
in Connect(Status) (created by OrganizationWrapper)
in Suspense (created by OrganizationWrapper)
If you do, can you look into whether this was there since before or not? If not please fix it, if it was already there please create an issue.
| // return 404 for invalid organization slug or org not listed in config | ||
| if (!validSlug) { | ||
| res.status(404).type("application/json").send({ | ||
| response_code: "INTERNAL_SERVER_ERROR", |
There was a problem hiding this comment.
Probably we don't want to send "INTERNAL_SERVER_ERROR" here but something different.
If this case is triggered, we must make sure the old behavior of sending the SMS token anyway is not triggered in this case because we know it's not needed.
pandafy
force-pushed
the
issues/656-do-not-send-sms
branch
from
b20bb74 to
590b6b9
Compare
| if (error.response && error.response.status === 404) { | ||
| // This is kept for backward compatibility with older versions of OpenWISP RADIUS | ||
| // that does not have API endpoint for checking phone token status. | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The backend was already returning 404 response when the API endpoint is not present in OpenWISP RADIUS. I added this handling in the frontend.
This bug is also present on the master branch. I opened #668 |
pandafy
force-pushed
the
issues/656-do-not-send-sms
branch
from
590b6b9 to
3b4c660
Compare
pandafy
force-pushed
the
issues/656-do-not-send-sms
branch
from
3b4c660 to
d53b576
Compare
| } | ||
| return org.slug === reqOrg; | ||
| }); | ||
| // Return 401 for invalid organization slug or org not listed in config. |
There was a problem hiding this comment.
Is this comment right? Is 401 used here?
There was a problem hiding this comment.
Earlier, I used 401 here. But, that would not be correct. So, I updated the code to return 404 and used a different response code.
Closes #656