search

nus-cs2103-AY2324S2 / forum

16 stars 0 forks source link

Would this situation be a feature enhancement or a bug fix? #779

Closed STELLA-LYE closed 7 months ago

STELLA-LYE commented 7 months ago

My team have some doubts regarding CS2103T v1.4 feature freeze and would like to clarify them.

My team would like to make the following fixes to our app but are unsure if they would violate the feature freeze constraint. The changes planned are as follows:

  1. Update the email template generation feature to throw an error when a non-existing group is provided.
  2. Update the email template generation feature to throw an error when an existing group has no students.
  3. Update the email template generation feature to throw an error and check whether the existing group provided has been assigned their specific telegram link.

One of my team's application features is to generate an email template for the user. However, in the User Guide we have specified that it is required for the parameter to be a "valid existing group" but our app was unable to catch this constraint for v1.3. Therefore, we were wondering if change (1) is a valid bug fix. However, we are unsure for changes (2) and (3).

The following is the link to the pull request changes made and our user guide document: https://github.com/AY2324S2-CS2103T-W10-4/tp/pull/131

damithc commented 7 months ago

@STELLA-LYE can you elaborate the current behavior for 1,2,3 is strictly incorrect (i.e., not can-be-better, but incorrect)? For example, why is it incorrect to generate an email template for an empty group?

STELLA-LYE commented 7 months ago

In our User Guide, we have specified that it is required for the parameter to be a "valid existing group". In addition, we think that it would be displaying incorrect information to users, to have an email template for a group that does not exist, an empty group and a group with no telegram link assigned.

However, we are unsure if our assumption above are correct. Thank You!

damithc commented 7 months ago

@STELLA-LYE We don't allow tightening validity checks unless the current level of validity checks cause serious problems (see Q5 of https://nus-cs2103-ay2324s2.github.io/website/schedule/week12/project.html). This case doesn't seem to qualify. So the fix should have been to just update the UG to match the current behavior and caution the user to use valid groups only.