Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix semiannually and semimonthly period calculations #3272

Merged
merged 8 commits into from
Nov 16, 2020

Conversation

cemalettin-work
Copy link
Contributor

@cemalettin-work cemalettin-work commented Oct 30, 2020

"semiannualy" and "semimonthly" periods were calculated incorrectly, this pr fixes those.
For "semiannualy", previous code would produce an incorrect period if you are at 2nd or 4th quarters currently:

  [RECURRENCE_TYPE.SEMIANNUALY]: (date, offset) => ({
    start: date
      .clone()
      .subtract(2 * offset, "quarters")
      .startOf("quarter"),
    end: date
      .clone()
      .subtract(2 * offset, "quarters")
      .endOf("quarter")
  }),

Example input: date = 30 October ( It shouldn't start from Octobers or Aprils, only 1 Jan or 1 July)
semianually-wrong

For "semimonthly", it is also incorrect, same input date, it misses first half of the months completely:
semmonthly-wrong

Fixed versions' behaviour:

  • Semimonthly => (1st - 14th) -> (15th - end of Month)
  • Semiannualy => (1 Jan - 30 June ) -> ( 1 July - 31 December)

User changes

  • Users now can see correct periods when there are "semiannualy" and "semimonthly" assessment periods

Super User changes

Admin changes

System admin changes

  • anet.yml or anet-dictionary.yml needs change
  • db needs migration
  • documentation has changed
  • graphql schema has changed

Checklist

  • Described the user behavior in PR body
  • Referenced/updated all related issues
  • commits follow a repo#issue: Title title format and these 7 rules
  • commits have a clean history, otherwise PR may be squash-merged
  • Added and/or updated unit tests
  • Added and/or updated e2e tests
  • Added and/or updated data migrations
  • Updated documentation
  • Resolved all build errors and warnings
  • Opened debt issues for anything not resolved here

@cemalettin-work cemalettin-work force-pushed the fix-semiannualy-and-semimonthly branch from b69d819 to e7e9f14 Compare October 30, 2020 07:47
@VassilIordanov VassilIordanov self-requested a review October 30, 2020 08:28
Copy link
Contributor

@VassilIordanov VassilIordanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow template for PRs and in it describe what is wrong with the current behavior and what the proposed new behavior is

@cemalettin-work cemalettin-work changed the title Fix semiannualy and semimonthly Fix semiannualy and semimonthly period calculations Oct 30, 2020
client/src/periodUtils.js Outdated Show resolved Hide resolved
client/src/periodUtils.js Show resolved Hide resolved
@cemalettin-work cemalettin-work force-pushed the fix-semiannualy-and-semimonthly branch from e7e9f14 to e8cbab3 Compare October 30, 2020 14:24
client/src/periodUtils.js Show resolved Hide resolved
@cemalettin-work
Copy link
Contributor Author

I will write a couple of unit tests for semimonthly and semiannually, as they are the most complicated ones.

@cemalettin-work cemalettin-work force-pushed the fix-semiannualy-and-semimonthly branch from b547063 to 88d38b2 Compare November 2, 2020 12:23
client/tests/utils/periodUtils.test.js Outdated Show resolved Hide resolved
@cemalettin-work cemalettin-work changed the title Fix semiannualy and semimonthly period calculations Fix semiannually and semimonthly period calculations Nov 2, 2020
@cemalettin-work cemalettin-work force-pushed the fix-semiannualy-and-semimonthly branch from 88d38b2 to 601b6b7 Compare November 2, 2020 13:58
@gjvoosten gjvoosten force-pushed the fix-semiannualy-and-semimonthly branch from b394571 to bc9825e Compare November 2, 2020 15:37
@cemalettin-work cemalettin-work force-pushed the fix-semiannualy-and-semimonthly branch from bc9825e to 1a74a4b Compare November 3, 2020 07:18
@gjvoosten gjvoosten requested a review from maradragan November 4, 2020 08:23
@cemalettin-work cemalettin-work force-pushed the fix-semiannualy-and-semimonthly branch from 1a74a4b to dc5a11e Compare November 4, 2020 08:35
client/src/periodUtils.js Outdated Show resolved Hide resolved
client/src/periodUtils.js Outdated Show resolved Hide resolved
@cemalettin-work cemalettin-work force-pushed the fix-semiannualy-and-semimonthly branch from be9356a to 5b7b912 Compare November 10, 2020 06:50
@VassilIordanov
Copy link
Contributor

Obviously anet.yml doesn't need change, but anet-dictionary.yml does (we need to update the PR template)

@VassilIordanov VassilIordanov merged commit c10dd57 into candidate Nov 16, 2020
@VassilIordanov VassilIordanov deleted the fix-semiannualy-and-semimonthly branch November 16, 2020 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants