search

GENESIIS / campus

0 stars 0 forks source link

C5-public : browse Category landing page : handle ajax request to display programs according to the category #6

Open HimashaFernando opened 7 years ago

HimashaFernando commented 7 years ago

Business benefit: This use case defines the common landing page for the public user, who browsers 'Corporate training'. The elements needs to be arrange within the UI in a proper way by considering user friendliness.

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

Miyuru-Genesiis commented 7 years ago

201610251219 - MM

Created git branch, created sample page and carried out initial commit.

Miyuru-Genesiis commented 7 years ago

Initial mock-up for coporate training landing page. Adapted from mock-up for higher education. To be updated.

initial mock-up for coporate training landing page adapted from higher education

Miyuru-Genesiis commented 7 years ago

Updated design for mock-up for corporate training landing page. In line with the mock-up for higher education (to ensure that the site design is kept homogeneous). c5-corporate-training-landing-page mockup

Miyuru-Genesiis commented 7 years ago

201610270235 - MM

Miyuru-Genesiis commented 7 years ago

201610270645 - MM

Miyuru-Genesiis commented 7 years ago

201610271856 - MM

Miyuru-Genesiis commented 7 years ago

201610270446 - MM

Miyuru-Genesiis commented 7 years ago

201610270845 - MM

Miyuru-Genesiis commented 7 years ago

201610271335 - MM

Miyuru-Genesiis commented 7 years ago

201610281755 - MM

Miyuru-Genesiis commented 7 years ago

201610310855 - MM

Miyuru-Genesiis commented 7 years ago

201610311120 - MM

Miyuru-Genesiis commented 7 years ago

201610311835 - MM

Miyuru-Genesiis commented 7 years ago

201611011830 - MM

Miyuru-Genesiis commented 7 years ago

201611020835 - MM

Miyuru-Genesiis commented 7 years ago

201611021325 - MM

Miyuru-Genesiis commented 7 years ago

201611021328 - MM

c5-corporate-training-landing-page Issue moved to QA.

Miyuru-Genesiis commented 7 years ago

201611021505 - MM

Chamindana commented 7 years ago

CO1103-CN

QA Review summary

1.Header comment is not mentioned for methods and class sections in the source files as follows.

CorporateProgrammeDAO.java CourseProviderProgrammeDAO.java CmdListCorporateProgrammes.java Programme.Java CorporateTraining.jsp

2.When click on Submit button without data in text box,then it is going to hit an Exception :

com.genesiis.campus.controller.CampusController process(): :java.lang.NumberFormatException: For input string

3.When click on Submit button with Invalid data range(eg:35353353>>,;;'[]1) in text box,then it is going to hit an Exception :

com.genesiis.campus.controller.CampusController process(): : java.lang.NumberFormatException: For input string

Assigning back to @Miyuru-Genesiis for a MX fix

jayani1991 commented 7 years ago

201611071705 -JH

Miyuru-Genesiis commented 7 years ago

201611081156 - MM

Due to changes made to the framework classes to ensure ability to respond in JSP as well as JSON form (and to preserve the hitherto developed code base), further dev changes, from now on-wards, will be pushed to a newly created branch named "c5-corporate-training-landing-page-MP-mm".

Updated estimation is below:

Miyuru-Genesiis commented 7 years ago

201611081435 - MM

Miyuru-Genesiis commented 7 years ago

201611090635 - MM

Miyuru-Genesiis commented 7 years ago

201611091427 - MM

Miyuru-Genesiis commented 7 years ago

201611100620 - MM

Miyuru-Genesiis commented 7 years ago

201611100632 - MM

Miyuru-Genesiis commented 7 years ago

201611101330 - MM

Miyuru-Genesiis commented 7 years ago

201611101750 - MM

Miyuru-Genesiis commented 7 years ago

201611111122 - MM

Miyuru-Genesiis commented 7 years ago

201611112145 - MM

Miyuru-Genesiis commented 7 years ago

201611121442 - MM

Miyuru-Genesiis commented 7 years ago

201611130106 - MM

Miyuru-Genesiis commented 7 years ago

201611141521 - MM

Miyuru-Genesiis commented 7 years ago

201611150918 - MM

Miyuru-Genesiis commented 7 years ago

201611151335 - MM

Miyuru-Genesiis commented 7 years ago

201611151353 - MM

Issue moved to Code Review

pabodhaW commented 7 years ago

201611151640 PN Code review comments Improvements

  1. CourseProvider.java class has no header comments.
  2. CmdListCategoryProgrammes.execute() method, exception handling violates the main flow of xeno:25. I think the validations that developer has done with exception handling needs to modify.Also, I think It's better if the responsibilities given to the execute () method, can be minimized. This applies for CmdListCategoryLandingPage command class and DAO classes too. KTD

@Miyuru-Genesiis FYI

Miyuru-Genesiis commented 7 years ago

201611161325 - MM

Carried out modifications recommended in previous code review.

  1. Added header comment in CourseProvider.java class
  2. Overhauled validation and exception-handling mechanism in execute() method in CmdListCategoryProgrammes.java so that it conforms to style used in xeno:25. Moved validation-related code out to new method defined in Validator class.

Please note that code in CmdListCategoryLandingPage.java is not developed as part of this issue - it is maintained under c7 - and the file here does not reflect the latest code as appearing in c7. Modifications may well have been carried out in that branch relating to that class, which may be put through it its own code review.

Issue assigned again to @pabodhaW for Code Review.

pabodhaW commented 7 years ago

201611161439 PN Code review issue fixes rtc 201611161325 - MM. @Miyuru-Genesiis has fixed all the issues mentioned in rtc 201611151640 PN.

Therefore I'm moving this issue to the next stage, QA.

Miyuru-Genesiis commented 7 years ago

201611161632 - MM

c5-corporate-training-landing-page-MP - Removed unnecessary UI elements and pages to avoid any confusion during QA 70181ab

Miyuru-Genesiis commented 7 years ago

201611161632 - MM

Testing Guidelines

Scope:

Changed/added Files: web/index.jsp web/dist/partials/categoryLandingPage.jsp web/dist/js/categoryLandingPage/categoryLandingPage.js

CmdListCategoryProgrammes.java CategoryProgrammeDAO.java SystemConfigDAO.java Programme.java CourseProvider.java Category.java EducationCategory.java Operation.java SystemConfig.java Validator.java

Grade of risk of change Low

Sprint released Sprint 1

Unit Test Done

How can it be tested

  1. Navigate to domain (localhost:8080). Click link "Click here to proceed"
  2. Enter a number indicating the category code (this must match records in Category table in DB)
  3. categoryLandingPage.jsp will load with empty areas on the right (assigning content to these areas is the task of a different issue). Ajax request will be sent on completion of loading the page, and programmes, filtering items and pagination controls will appear on the page.

What will it affect Display of Programmes for a category

What user roles undertake this Public user

Other comments Tested on Google Chrome - version 54.0.2840.99 m

Please note that this issue contains code written for issue c7 (esp. CmdListCategoryLandingPage.java and CategoryCourseProviderDAO.java) which is not maintained as part of this issue.

tharakaGen commented 7 years ago

campus-corporate-traing-landing

HimashaFernando commented 7 years ago

201611211955 HF - Local test cycle 1 summary

1. 'java.lang.NumberFormatException' exception is given for an empty string input from 'dummyPage.jsp'

19:48:19,655 ERROR [com.genesiis.campus.controller.CampusController] (http-0.0.0.0:8080-4) process(): Exception : java.lang.NumberFormatException: For input string: "" ... at com.genesiis.campus.command.CmdListCategoryLandingPage.execute(CmdListCategoryLandingPage.java:62) [campus.jar:] at com.genesiis.campus.util.DataHelper.getResultView(DataHelper.java:99) [campus.jar:] at com.genesiis.campus.controller.CampusController.process(CampusController.java:74) [classes:] at com.genesiis.campus.controller.CampusController.doPost(CampusController.java:60) [classes:] at com.genesiis.campus.publicController.PublicController.doPost(PublicController.java:46) [classes:]

2. Category landing page is not related. For all inputs from dummyPage.jsp, the page title is same.

bug02_c5

3. Testing guideline says 'Majors will be shown for CorporateTraining category' in orange color area as filters but it is now shown levels as filtering option. What will be the correct one?

bug03_c5

4. Testing guideline says ''Programmes should be shown in the middle of the page with a pagination control (a set of numbered buttons) just below it (maximum number of programmes displayed for a "page" - when a single such paginator button is clicked - is 20).''

But, there are only top 10 records are selected according to 'CategoryProgrammeDAO.java' query string. (If i am write)

SELECT p.*, cp.SHORTNAME, cp.UNIQUEPREFIX, cp.NAME AS COURSEPROVIDERNAME, cp.LOGOIMAGEPATH, ct.NAME AS CLASSTYPENAME, m.NAME AS MAJORNAME, l.NAME AS LEVELNAME, t.CODE AS TOWNCODE, t.NAME AS TOWNNAME FROM (SELECT TOP 10 NEWID() as dummy, a.CODE FROM (SELECT p.CODE FROM [CAMPUS].[PROGRAMME] p WHERE p.CATEGORY = 3 AND p.PROGRAMMESTATUS = 1) a ORDER BY NEWID()) b JOIN [CAMPUS].[PROGRAMME] p ON (p.CODE = b.CODE) JOIN [CAMPUS].[COURSEPROVIDER] cp ON (p.COURSEPROVIDER = cp.CODE) JOIN [CAMPUS].[CLASSTYPE] ct ON (ct.CODE = p.CLASSTYPE AND ct.ISACTIVE = 1) JOIN [CAMPUS].[MAJOR] m ON (m.CODE = p.MAJOR AND m.ISACTIVE = 1) JOIN [CAMPUS].[LEVEL] l ON (l.CODE = p.LEVEL AND l.ISACTIVE = 1) LEFT JOIN [CAMPUS].[PROGRAMMETOWN] pt ON (p.CODE = pt.PROGRAMME AND pt.ISACTIVE = 1) LEFT JOIN [CAMPUS].[TOWN] t ON (pt.TOWN = t.CODE) ORDER BY p.CODE;

If only 10 records are displaying, why we need pagination? If the function is implemented to display all corporate training programs, then that function has broken. Though i am having more than 20 valid records, still only 10 records are displayed. pagination is not working. I am little bit confused with this guideline and the implementation.

bug04_c5

5. I think user should get a message something like 'There are no any active corporate training programs' if all the programs have the status as 0(this is very rare situation but, still it should be handled). Now, the view is empty.

bug05_c5

6. Is it not needed to consider about the program expiry dates? Are we displaying expired programs as well in the list? What will be the program status for expired but not deactivated programs? KTD @Miyuru-Genesiis @tharinduw

Assigning the issue back to @Miyuru-Genesiis

Miyuru-Genesiis commented 7 years ago

201611221856 - MM

MX 1 Fix undertaken

Moved back to Code Review

pabodhaW commented 7 years ago

201611222140 PN Code review comments Improvements

  1. CmdListCategoryProgrammes.execute() method coding if (EducationCategory.CORPORATE_TRAINING.equals(educationCategory)) { // Consider Major data indexOfMajorOrLevelCode = 12; indexOfMajorOrLevelName = 23; filterType = "Major"; } else { // Consider Level data indexOfMajorOrLevelCode = 14; indexOfMajorOrLevelName = 24; filterType = "Level"; }

The above code block needs a proper explanation I think. It is confusing when reading the code. Also setting a static value to a variable needs to be well confirmed with the value, that it will not changed in extending the program.

FYI @Miyuru-Genesiis

Miyuru-Genesiis commented 7 years ago

201611240943 - MM

Made changes recommended in previous Code Review. Assigned to @pabodhaW to confirm the changes before pushing to QA.

pabodhaW commented 7 years ago

201611241150 PN Crev comments

@Miyuru-Genesiis has fixed the mentioned issue in rtc 201611222140 PN. Fix rtc 201611240943 - MM

No any other issues in crev. Therefore moving this issue to the next stage.

@Chamindana assigning you to proceed with QA.

Miyuru-Genesiis commented 7 years ago

201611241322 - MM

Modifications recommended in the previous QA have been carried out.

After comments in the previous QA, I think it is important to clarify the background of this issue and the work-flow surrounding it.

This issue was previously expected to handle the loading of the JSP as well as the subsequent Ajax-initiated loading of the list of programmes plus the list of filters (Majors or Levels) for only the Corporate Training category. The issues c4-school-education-landing-page and c7-higher-education-landing-page were identical to this issue, in that they were tasked with the same operations of initial JSP loading and subsequent Ajax-initiated loading of content, except they related to School Education and Higher Education categories respectively. After sometime into development, it was obvious that there was significant duplication of effort to write almost the same code for all three branches, and there was a requirement that the same JSP file had to be used to load content for all categories. Here, I and the developer for other two branches (c4 and c7), JH, decided to split the work in such a way that she would handle the loading of the JSP page according to the categoryCode sent from someplace - without the programme list - as part of c7, and I would code the part where the Ajax request is sent after the page is loaded so the list of programmes for the relevant category is fetched and displayed properly in the relevant page area.

Point 1 in QA comments: NumberFormatException: So, it should be clear now that the loading of the JSP page - as response to some request containing the categoryCodeas the parameter - is handled as part of c7. The issue cited under point 1 in the QA review, in fact, concerns this functionality of loading the JSP page. I believe this is implemented properly as part of c7. I kept a bare-necessary amount of code from c7 here for this issue - just enough to load the JSP - so I can load the programme list via Ajax on that JSP page.

So, basically, handling validation to prevent the exception you cited is not part of this issue.

Point 2 in QA comments: Page title not reflecting the current category: For point 2 of the QA review as well, the above applies. Display of category details on the loading of the JSP page is handled under issue c7, and that area of the page is not manipulated as part of the process of displaying the list of programmes via the Ajax request, which is the only task c5 now deals with.

Point 3 in QA comments: Levels being displayed instead of Majors: On point 3 of the review, let me draw your attention to lines 89 - 100 on CmdListCategoryLandingPage.java. You would remember that, in your database, the value for CODE field in CATEGORYtable for the conceptual Corporate Training category is 3. Therefore, it would not be difficult to see that after the switch construct is run in those lines of code, the variable categoryIdentifierString would have HIGHER_EDUCATION as the value when the categoryId has the value 3.

That is the reason why, when you were testing, the system showed Levels, not Majors, even though it was programmes of Corporate Training category that were being loaded. Now, the reason why this mapping of category-code to "category-identifier-string" was hard-coded like this is because it was only later that it was identified that it would be better to have another field in the CATEGORYtable, named CATEGORYSTRING, that would map to constants in the SystemConfigenum class (in com.genesiis.campus.validation package) and it was a temporarily solution for that moment until the DB table was updated to have that column. Also, again, code in CmdListCategoryLandingPage.java is maintained as part of issue c7, and it is likely to be correctly implemented there.

Please also look at lines 128in categoryLandingPage.jsp and lines 106 - 113 and 138 - 148 in CmdListCategoryProgrammes.java, if interested, on how this value that is assigned for variable categoryIdentifierString in CmdListCategoryLandingPage.javaclass is used.

Point 4 in QA comments: TOPclause preventing the loading of more than 10 programmes: On point 4 of the QA review, an issue was indeed there, which is now fixed. The TOPclause that was there in the query, a remnant of the earlier implementation, is now removed, and now the pagination control displays the correct number of buttons when the size of the result-set exceeds 20.

Point 5 in QA comments: Feedback to user and other page behaviour when no results are displayed: The issue highlighted on point 5 in the QA review has also been fixed; now a message displays when there are no programmes returned from the server, informing the user of that fact.

Point 6 in QA comments: Considering only programmeStatus and disregarding expirationDate in query: On point 6 of the QA review, my understanding was that a yet-to-be-implemented Scheduler mechanism would look at things like Expiry Dates in records and would change the values assigned for "status" fields; and we only look at the these status fields in our queries (we do not look at expirationDate because we assume the current value in programmeStatus is there after the Scheduler has accounted for the value for expirationDate in that record). In the absence of a convention on the value-set for programmeStatus field when the issue was being developed, I assumed that value 1 for programmeStatus would indicate the particular state of active, non-expired programmes. The query will be updated accordingly when this convention for programmeStatus values is formulated.

Open for discussion @HimashaFernando

HimashaFernando commented 7 years ago

201611241730 HF

re 201611241322 - MM : explanation of issue background

@Miyuru-Genesiis Thanks for the good explanation and it is very helpful. Also, it would have been more useful if you and @jayani1991 commented these issue scope refactoring at the same time it took place. Also the most important thing that we all should aware is 'the issue title'. It would be better if the person who handling the issue can refactor the issue title as well according to the issue scope changes and reqm changes since the very first detail anyone can get the idea about the issue is 'the title'.

I have updated the issue title as below, old title : C5-public : browse Corporate training landing page : create the page new title : C5-public : browse Category landing page : handle ajax request to display programs according to the category

@Miyuru-Genesiis check that out and update if i am wrong. :-) @jayani1991 better to refactor c7 issue title also.

I think since the category page reqm are covered from c5 and c7, c4 is no longer a valid issue. It would be better to remove c4 from the issue list. KTD @Miyuru-Genesiis @jayani1991 @tharinduw

HimashaFernando commented 7 years ago

201611242050 HF - QA Cycle 2 Summary

Point 1 & 2 Agreed. I tested these facts in c7.

Point 3 in QA comments: Levels being displayed instead of Majors: Understood. Thanks for the explanation. I tested this function in c7 and it is working as expected. I verified your implementation also, by passing the value as 4(hard coded value for corporate training), and I got the major filter.

Point 4 in QA comments: TOPclause preventing the loading of more than 10 programs: Since the top clause is removed, total result is displaying with pagination now. But, an empty pagination is displayed when (queried total record count / 20) >= x.5 [x=0, 1, 2, .......]

example: when queried total record count has 29 records, it will display 2 pagination. That is correct. When queried total record count has 30 or above value, it will display 3 pagination with empty last page.

bug04_c5

Point 5 in QA comments: Feedback to user and other page behaviour when no results are displayed: Tested and verified. It is fixed now.

Point 6 in QA comments: Considering only programmeStatus and disregarding expirationDate in query: Agreed with you. I discussed about this point with JH today. Yes we can use that scheduler mechanism later and assume that program status 1 is only referring active and not expired programs.

Point 7 - a new suggestion Can we display the displaying program count out of total program count in each pagination? (something like 20/46, 20/46, 6/46) :-)

Assigning back to @Miyuru-Genesiis and moving to MX to check point 4, 7.