search

GENESIIS / campus

0 stars 0 forks source link

C8-public : make inquiry for course : create inquiry form and link with the relevant UI elements #11

Open HimashaFernando opened 8 years ago

HimashaFernando commented 8 years ago

Business benefit: User can inquire for course. It's easy for user to get details about the course at any time.

Estimation: UI - 32sp Dev - 48sp Testing - 48sp

anuradha999 commented 8 years ago

inquary

anuradha999 commented 8 years ago

201610261720 AS pushed 2 commits to campus:c8-inquiry-form-for-course-as

anuradha999 commented 8 years ago

201610271745 AS pushed 10 commits to campus:c8-inquiry-form-for-course-as

anuradha999 commented 8 years ago

201610280715 AS pushed 4 commits to campus:c8-inquiry-form-for-course-as

anuradha999 commented 8 years ago

201610281215 AS pushed 1 commit to campus:c8-inquiry-form-for-course-as

anuradha999 commented 8 years ago

201610311305 AS

pushed 4 commits to campus:c8-inquiry-form-for-course-as

anuradha999 commented 8 years ago

201611011735 AS pushed 4 commits to campus:c8-inquiry-form-for-course-as

anuradha999 commented 8 years ago

201611021715 AS Pushed 4 commits to campus: c8-inquiry-form-for-course-as

anuradha999 commented 8 years ago

201611031620 AS pushed 5 commits to campus:c8-inquiry-form-for-course-as

anuradha999 commented 8 years ago

201611081125-AS Due to the design change that's intended to support both Jstl and JSON data , through the same CampusController.java (servlet) and the decision to follow the multi-page Application approach instead of SPA (Single Page Application), the rest of the code progression will be conducted in c8-inquiry-form-for-course-MP-as branch.

anuradha999 commented 8 years ago

201611091705-AS pushed 17 commits to campus:c8-inquiry-form-for-course-MP-as

anuradha999 commented 8 years ago

201611101725-AS pushed 1 commit to campus:c8-inquiry-form-for-course-MP-as

anuradha999 commented 7 years ago

201611151145-AS pushed 5 commits to campus:c8-inquiry-form-for-course-MP-as

anuradha999 commented 7 years ago

201611151145-AS c8-inquiry-form-for-course-MP-as developing complete move to Code review.

pabodhaW commented 7 years ago

201611151500 PN Code review comments Improvements

  1. Remove the logger statement in InstituteController.doGet() method and Validator.validateCourseInquiry(StudentProgrammeInquiry data) method. log.info("Test");
  2. InstituteController.java add default serialVersionUID in the beginning. private static final long serialVersionUID = 1L;
  3. Path reference is incorrect for the validator.js class. <script src='../../../../dist/js/institute/validation/validation.js'></script> use this format and developer can reuse the JavaScript validations.
  4. GeneralMail.setProperties() method not throwing an exception. Add a throw statement.
  5. Remove unused imports. InstituteCmdFactory.java, PublicCmdFactory.java
  6. Inside DAO class there is a String instance for assign the query. But again, while setting it into the preparestatement, developer convert again into a String. String query = "INSERT INTO CAMPUS.STUDENTPROGRAMINQUIRY ( NAME, EMAIL, TELEPHONECOUNTRYCODE, TELEPHONEAREACODE, TELEPHONENUM, INQUIRYTITLE, INQUIRYTEXT, INQUIRYDATE, INQUIRYTIME, STUDENT, PROGRAMME, ISACTIVE, CRTON, CRTBY) VALUES( ?, ?, ?, ?, ?, ?, ?, getdate(), getdate(), ?, ?, '1', getdate(), 'admin') "; ps = conn.prepareStatement(query.toString()); I think this is not necessary. KTD.
  7. It's better if developer can assign SQL query into a string before pass it into a PreparedStatement. preparedStatement = conn .prepareStatement("SELECT COURSEPROVIDER.COURSEINQUIRYEMAIL FROM [CAMPUS].[COURSEPROVIDER] INNER JOIN [CAMPUS].[PROGRAMME] ON COURSEPROVIDER.CODE = PROGRAMME.COURSEPROVIDER WHERE PROGRAMME.CODE = ?;");
  8. Methods other than execute() in CmdSendCourseInquiry.java class move into another class. Then the methods can be reuse later. It's better if the object declarations can be removed from he command class.

@anuradhars FYA.

anuradha999 commented 7 years ago

201611151820 AS pushed 6 commits to campus:c8-inquiry-form-for-course-MP-as

anuradha999 commented 7 years ago

201611151820 AS Code review issues - fixed

  1. Remove the logger statement in InstituteController.doGet() method and Validator.validateCourseInquiry(StudentProgrammeInquiry data) method. log.info("Test"); - Fixed
  2. InstituteController.java add default serialVersionUID in the beginning. private static final long serialVersionUID = 1L; - Fixed
  3. Path reference is incorrect for the validator.js class. use this format and developer can reuse the JavaScript validations. - **Fixed**
  4. GeneralMail.setProperties() method not throwing an exception. Add a throw statement. - KTD with @chathuriM and @dushanthacb

the error is logged at the place it would be generated. since there is no ripple effect on the flow I would think it's not necessary to throw the exception as it cause many methods to be changed at upper level

  1. Remove unused imports. InstituteCmdFactory.java, PublicCmdFactory.java - Fixed
  2. Inside DAO class there is a String instance for assign the query. But again, while setting it into the preparestatement, developer convert again into a String. String query = "INSERT INTO CAMPUS.STUDENTPROGRAMINQUIRY ( NAME, EMAIL, TELEPHONECOUNTRYCODE, TELEPHONEAREACODE, TELEPHONENUM, INQUIRYTITLE, INQUIRYTEXT, INQUIRYDATE, INQUIRYTIME, STUDENT, PROGRAMME, ISACTIVE, CRTON, CRTBY) VALUES( ?, ?, ?, ?, ?, ?, ?, getdate(), getdate(), ?, ?, '1', getdate(), 'admin') "; ps = conn.prepareStatement(query.toString()); I think this is not necessary. KTD. - Fixed
  3. It's better if developer can assign SQL query into a string before pass it into a PreparedStatement. preparedStatement = conn .prepareStatement("SELECT COURSEPROVIDER.COURSEINQUIRYEMAIL FROM [CAMPUS].[COURSEPROVIDER] INNER JOIN [CAMPUS].[PROGRAMME] ON COURSEPROVIDER.CODE = PROGRAMME.COURSEPROVIDER WHERE PROGRAMME.CODE = ?;"); - Fixed
  4. Methods other than execute() in CmdSendCourseInquiry.java class move into another class. Then the methods can be reuse later. It's better if the object declarations can be removed from he command class. - KTD with @chathuriM and @dushanthacb for code refactor. All of these code which are converted to the method are originally in the execute() method. In order keep Single responsibility principle SRP ,I have encapsulate the responsibilities in to methods and these methods are specific to the class itself hence hard to reused in a separate class according to MHO

And move to code review @pabodhaW

pabodhaW commented 7 years ago

201611152305 PN Code review issue fixes

rtc 201611151820 AS Other than 4 and 8 all the other mentioned issues are fixed. Till this fix also done c8-inquiry-form-for-course-MP-as moving back to the MX.

@anuradhars FYA.

dushanthacb commented 7 years ago

201611161620-DN Reasoning for item # 4 and 8

  1. GeneralMail.setProperties() method not throwing an exception. Add a throw statement. - KTD with @chathuriM and @dushanthacb

the error is logged at the place it would be generated. Since there is no ripple effect on the flow, I would think it's not necessary to throw the exception as it causes many methods to be changed at upper level

8 Methods other than execute() in CmdSendCourseInquiry.java class move into another class. Then the methods can be reuse later. It's better if the object declarations can be removed from he command class. - KTD with @chathuriM and @dushanthacb for code refactor.

All of these code which are converted to the method are originally in the execute() method. In order keep Single responsibility principle SRP ,I have encapsulate the responsibilities in to methods and these methods are specific to the class itself hence hard to reused in a separate class according to MHO

pabodhaW commented 7 years ago

201611161641 PN Code review issue fixes rtc 201611161620-DN. Considering the comment given by @dushanthacb I'm moving this issue to the next stage, QA.

anuradha999 commented 7 years ago

201611161936 AS

What has been done(Scope)

Send Inquiry for Course This function should integrate in the mini web. In system, student logged into the institute mini web and select the course details and Click the course inquiry button. Then after redirect to the course-inquiry. jsp . (In time bean course-inquiry.jsp link in menu NEWS tab) In system, logged studentID and Student select program ID get from session, in time bean student code and program code hard code in hidden tags.

Change/Add Files CmdSendCourseInquiry.java CourseInquiryDAO.java CourseProviderDAO.java StudentProgrammeInquiry.java InstituteCmdFactory.java CaptchaResponse.java MailServerManager.java ReCaptchaManager.java EmailDispenser.java GeneralMail.java IEmail.java ResponseType.java Validator.java SystemMessage.java institute.helper.js validation.js course-inquiry.jsp InstituteController.java

Grade of risk of change Low

Sprint released Sprint 1

Unit Test Done

How can it be tested Browse : localhost:8080 or www.campus.dev

What will it affect Logged in Student can send an inquiry about the Course.

How can it be tested Click on News tab on site map

What user roles are affected student

Other comments Use Number 1 as a Student Code. (Jsp Hidden tag) Use Number 7 as a Program Code. (Jsp Hidden tag) reCapture function only works on localhost:8080 Decrease the security level if email is not working

anuradha999 commented 7 years ago

Sample output page of the issue : c8-inquiry-form-for-course-MP-as

ui_course_inquiry_form

HimashaFernando commented 7 years ago

201611221615 HF - Local test cycle 1 summary

1. course-inquiry.jsp page loads gave following errors.

bug01_c8

2. Empty field validation error messages, successful messages and etc should be more prominent. (better to use different color, different font from the field labels)

bug02_c8

3. When an auto fill value from suggestion list is selected to a field, the field display different from other fields. Would it be fine?

bug03_c8

4. Use case fails for an invalid email.

steps to recreate : go to inquiry page > fill all other details with valid data except email field > add '123' to email field > give a valid recapture > send message

actual output : successful message displays, server side says invalid email, email does not receive

bug04_c8

expected output : email field should be validated for valid email address, user should get an error message

5. Use case fails for an invalid invalid telephone number (ex; 'sd3').

Same output in the point 4 is observing for above invalid telephone number. I guess server side validation has done for letters(not sure, just a guess) but telephone number should be validated from both server side and client side. (only numeric is valid). Below are the valid formats, +94774372661 774372661 0774372661 +94 77 4372 661 +94 77 437 2661

6. When server side got errors on sending emails(something like issues on mail servers), user should get a proper error message saying the 'inquiry has not been sent due to an internal error'(message is only a suggestion). This has not been handled. User is getting below meaningless error.

bug06_c8

I tested this by changing the host name.

7. When only the recapture field is wrong, all other fields are getting reset and user gets an error message. It is not user friendly. Each and every time user misses to click on recapture, he/she have to fill all the details again. The error message is also meaning-less. This should be fixed

steps to recreate: go to contact us > fill all the fields with valid data except clicking on the recapture > click on submit

bug07_c8

Suggestions S1. It would be better if the email body has full name, email address, phone number of the sender since we are collecting those inputs from the user.

S2. I suggest to remove country code and area code. I feel like it's not needed. KTD @anuradhars @tharinduw

Note : tested only in Google chrome.

Assgning back to developer @anuradhars

tharinduw commented 7 years ago

all - discussion needed. As per KF we are restricting this functions only to logged users. Most of the fields can be preloaded to the form. (if needed can load data with editable text)

when sending inquiry, we will save all data in inquary table. (data will duplicate) but in case we allow non registered users to use this function, we can facilitate it.

TBD before s/up tomorrow.

anuradha999 commented 7 years ago

201611241710 AS

pushed 1 commit to campus:c8-inquiry-form-for-course-MP-as

anuradha999 commented 7 years ago

201611241720 AS Modified inquiry form for course UI.

inquary v2

c8-inquiry-form-for-course-MP- Done the QA modifications mentioned by @HimashaFernando
rtc 201611221615.HF Assign @chathuriM for code review.

chathuriM commented 7 years ago

201611251133 CM

  1. Remove Unused loggers in below classes 
    RecatpchaManager.java
Validator.java
CmdSendCourseInquiry.java
  2. Remove Unused imports in InstituteController servlet.
  3. Remove jsp and html pages that used for testing purposes. 
    index1.jsp
index.html
  4. In a CourseProviderDAO class there are ArrayList named as ArrayList<String> singleEmployeeList and Collection named as final Collection<String> singleEmployeeCollection. This issue is not related to employee. Its better to use meaning full variable names.
  5. In the CmdSendCourseInquiry class, there are set of unused variables. 
    private String fullname;
private String countryCode;
private String areaCode;
private String telNo;
private String inquiryTitle;
private int studentCode;
private int programmeCode;
  6. Remove unwanted comments in ReCaptcha Class.
  7. Remove unused class named as CmdGenerateEmail.
  8. In a CourseInquiryDAO class, It's better to check connection object is already created before close it. 
    if (conn != null) {
conn.close();
}

    @anuradhars FYA.

anuradha999 commented 7 years ago

201611251550 AS rtc 201611251133 CM code review issues fixed

pushed 1 commit to campus:c8-inquiry-form-for-course-MP-as

Moving this to QA stage and assigning @HimashaFernando

pabodhaW commented 7 years ago

201611281600 PN Crev comments

rtc 201611251550 AS rtc 201611251133 CM

I have noted an incorrect logger class import in InstituteController.java. import java.util.logging.Logger; So the fix for that is mentioned in below commit into log4j. import org.apache.log4j.Logger;

pabodhaW pushed 1 commit to campus:c8-inquiry-form-for-course-MP-as

Moving this to QA stage and assigning @HimashaFernando

HimashaFernando commented 7 years ago

201611281700 HF - QA cycle 2 summary

point 1 Invalid email validation is redirected to 'NO-DATA' error validation.

steps: fill details with invalid email(other fields should be valid) > click send message

actual output: invalid email error message is displayed and then page is redirected to 'No-Data' error message.

point 2 When the message is sent with only spaces in 'full name', 'inquiry title' and 'message' fields, the email is receiving with empty fields. Fields should be validated to not to enter spaces.

bug01_c8

Assigning back to @anuradhars

anuradha999 commented 7 years ago

201611291630 AS rtc 201611281700 HF code review issues fixed

pushed 1 commit to campus:c8-inquiry-form-for-course-MP-as

Assigning to @pabodhaW for code review.

pabodhaW commented 7 years ago

201611292145 PN rtc 201611291630 AS rtc 201611281700 HF

@anuradhars has fixed the points mentioned by @HimashaFernando.

There were few lines of comments in some classes. If those code blocks are not needed in future developer has to remove them from the code. Pointing that as an improvement. ( Remove them when merging. )

Since there are no more any critical issues in code, I'm moving this issue back to RFQA by assigning @HimashaFernando .

HimashaFernando commented 7 years ago

201611301313 HF - QA cycle 3 summary

Two points mentioned in comment '201611281700 HF - QA cycle 2 summary' was fixed. Tested and verified both two fixes. Moving to next stage 'to be merged' and assigning to @anuradhars since no more critical issues to report.