Fixes #65: Submit online application to Prof M School #78

lschaefer-sugarcrm · 2018-06-20T20:54:02Z

Add new fields for the application form
Update the Applicant record view layout
Create Online Application Form
Create page that is displayed when user submits application form
Update Postman Collection to create campaigns
Update Postman sample data to include application data
Add unit tests
Add acceptance tests
Add documentation

mmarum-sugarcrm · 2018-06-25T18:14:16Z

package/src/custom/src/application_campaign/ApplicationCampaignManager.php

+     *
+     * @return \SugarQuery
+     */
+    protected function getSugarQuery()


What's the motivation of this helper? It would seem to me like you'd want to build the SQL query in this method instead of in getOnlineApplicationCampaignId()?

I created this helper so that I would be able to mock the SugarQuery in automated tests. I followed a similar pattern in https://github.com/sugarcrm/school/blob/master/package/src/custom/modules/Contacts/Students_Gradebook.php.

Could we add a comment to PHPdoc that explains purpose?

Yes, updated

mmarum-sugarcrm · 2018-06-25T18:15:15Z

package/src/custom/src/application_campaign/ApplicationCampaignManager.php

+     *
+     * @return null|\SugarBean
+     */
+    protected function getNewCampaignBean()


Is this meant to be overridable? Is the idea we could support different types of SugarBeans?

Same idea for this one. I broke out the Campaign bean so I could mock it in automated tests.

Yes, updated

mmarum-sugarcrm · 2018-06-26T14:51:46Z

package/src/custom/src/application_campaign/ApplicationCampaignManager.php

+        $results = $q->execute();
+
+        if(sizeof($results) != 1 || !$results[0]['id']){
+            throw new \Exception("Unable to find ID for the Campaign named Online Applications");


Does this message get displayed to user? If so, we should consider using a language label.

Good question! I tried to find a way to display this message to the user, but I couldn't figure out how. No matter what I did, the page just displayed "We are sorry, the server is currently unavailable, please try again later." and the error message never displayed in the console. Is there a way to push a more informative message to the browser?

Yeah, you could echo a message/HTML here which should get displayed in browser. Throwing an exception will cause server to treat it like an HTTP error response. This exception message is likely only placed in php error log.

I played around with this some more. If I echo a message, it gets sent back as the Campaign ID, which isn't great. I decided to add some extra error handling to the Application Form to display an alert if the Campaign ID comes back empty (this would happen if someone installed Prof M without inserting the sample data).

If you convert this to a REST API then you can throw a SugarApiException or extend SugarAPiExcpeiont to create your own. That'll return a JSON encoded error message that'll allow a consumer to do error handling.

I've updated the exception and error handling.

- Add new fields for the application form - Update the Applicant record view layout - Create Online Application Form - Create page that is displayed when user submits application form - Update Postman Collection to create campaigns - Update Postman sample data to include application data - Add unit tests - Add acceptance tests - Add documentation

mmarum-sugarcrm · 2018-06-28T19:22:19Z

package/src/custom/online_application_form/ApplyOnline.html

+                }
+            });
+            // Get the ID of the Online Applications Campaign
+            xhr.open("GET", urlBase + "/index.php?entryPoint=getOnlineApplicationsCampaign");


Why is this an entrypoint? This is simply returning a record ID. If it's meant to be machine consumable then it should be a REST API endpoint. You can make it a public endpoint which should allow it to be accessible without an OAuth token. It should be relatively straight forward to convert to a public rest API.

I've updated the code to be an endpoint instead of an entrypoint. I've emailed @rhoggSugarcrm to see if I'm testing the endpoint in the best way.

I currently recommend using Thorn to test REST API endpoints, but it may be a bit of work to get it set up.

PHPUnit is certainly adequate until then.

mmarum-sugarcrm

One last nitpick.

mmarum-sugarcrm · 2018-07-09T19:42:11Z

package/src/custom/modules/Campaigns/clients/base/api/OnlineApplicationsAPI.php

+
+}
+
+?>


Remove pls. :)

Good catch! Updated.

May I make a recommendation (as the guy who was pinged on a totally unrelated issue and is now butting in because he didn't unsubscribe 😝)?

If you add some sort of linting checks to .travis.yml hopefully PHP closing tags and similar would be caught automatically. IMHO that's a best practice anyhow 😸

Lol - feel free to butt in! We have #41 on the backlog.

Remove entrypoint code and add tests Update Application Form to use REST endpoint Update Exception type Remove closing tag

lschaefer-sugarcrm requested a review from mmarum-sugarcrm June 20, 2018 20:54

lschaefer-sugarcrm force-pushed the issue-65 branch from d098dc7 to 5805fc5 Compare June 20, 2018 20:56

mmarum-sugarcrm requested changes Jun 26, 2018

View reviewed changes

lschaefer-sugarcrm added 2 commits June 27, 2018 15:52

Fixes #65: Update error handling and add phpdoc

a34a0b4

lschaefer-sugarcrm force-pushed the issue-65 branch from 5805fc5 to a34a0b4 Compare June 27, 2018 19:55

Fixes #65: Fix merge mistakes

68f3840

mmarum-sugarcrm requested changes Jun 28, 2018

View reviewed changes

lschaefer-sugarcrm force-pushed the issue-65 branch 2 times, most recently from 06c9be3 to a3bb8fc Compare July 3, 2018 14:24

mmarum-sugarcrm requested changes Jul 9, 2018

View reviewed changes

Fixes #65: Add custom endpoint to get Online Applications Campaign

2b32eb8

Remove entrypoint code and add tests Update Application Form to use REST endpoint Update Exception type Remove closing tag

lschaefer-sugarcrm force-pushed the issue-65 branch from a3bb8fc to 2b32eb8 Compare July 9, 2018 20:03

mmarum-sugarcrm approved these changes Jul 10, 2018

View reviewed changes

mmarum-sugarcrm merged commit 2362a8f into master Jul 10, 2018

lschaefer-sugarcrm deleted the issue-65 branch July 10, 2018 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #65: Submit online application to Prof M School #78

Fixes #65: Submit online application to Prof M School #78

lschaefer-sugarcrm commented Jun 20, 2018

mmarum-sugarcrm Jun 25, 2018

lschaefer-sugarcrm Jun 27, 2018

mmarum-sugarcrm Jun 27, 2018

lschaefer-sugarcrm Jun 28, 2018

mmarum-sugarcrm Jun 25, 2018

lschaefer-sugarcrm Jun 27, 2018

mmarum-sugarcrm Jun 27, 2018

lschaefer-sugarcrm Jun 28, 2018

mmarum-sugarcrm Jun 26, 2018

lschaefer-sugarcrm Jun 27, 2018

mmarum-sugarcrm Jun 27, 2018

lschaefer-sugarcrm Jun 28, 2018

mmarum-sugarcrm Jun 28, 2018 •

edited

Loading

lschaefer-sugarcrm Jul 3, 2018

mmarum-sugarcrm Jun 28, 2018

lschaefer-sugarcrm Jul 2, 2018

rhoggSugarcrm Jul 6, 2018

mmarum-sugarcrm left a comment

mmarum-sugarcrm Jul 9, 2018

lschaefer-sugarcrm Jul 9, 2018

rhoggSugarcrm Jul 9, 2018

lschaefer-sugarcrm Jul 9, 2018

Fixes #65: Submit online application to Prof M School #78

Fixes #65: Submit online application to Prof M School #78

Conversation

lschaefer-sugarcrm commented Jun 20, 2018

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

mmarum-sugarcrm Jun 28, 2018 • 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

mmarum-sugarcrm left a 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

mmarum-sugarcrm Jun 28, 2018 •

edited

Loading