Add named params and yard docs to `create_milestone` method at `GithubHelper` #426

raafaelima · 2022-11-06T16:44:32Z

What does this solve?

With the changes/improvements made on PR #420 and the creation of the unit tests being done on PR #425, we realized that the lack of yard documentation and named parameters were making the tests unclear and too verbose in the area of CreateMilestoneAction. So, as pointed on the PR #425 review, we decided to open this new PR to address these issues that were found.

How do the yard docs look now?

Glad you asked 😄

Dependency

This work is part of the one started on #425.

So, here at #426 we are addressing the changes at the API of GithubHelper#create_milestone, and there on #425 are the unit tests. As pointed on the #425 PR description, this should be merged first, as it affects the work done there.

…one method To become this method cleaner and more legible to call, we're adding named parameters to it, following the name of the current ones, and also adding yard docs to help guide the new people who use it

…ithubHelper's method

AliSoftware

I was a bit surprised that this PR didn't include the necessary changes to create_new_milestone_action_spec.rb on those lines.

Then I realized it was because that PR was targeting trunk (as opposed to being built on top of your raafaelima:add/unit-tests-for-milestone-actions branch from #425… which I guess you didn't do because you wouldn't have been able to open a PR in our repo otherwise, but instead only be able to open that PR on your fork?).

Would have been nice if you had mentioned that limitation / aspect in the description of the PR — or alternatively kept the PR as Draft (and commented on why) while you were waiting for #425 to land before un-drafting it — so that we wouldn't be surprised. Especially given that regardless if we merge #425 or this #426 first, you will have to adjust the tests in other one before we land it anyway (and as CI doesn't run on fork'd PRs by default for security reasons, it would be easy to accidentally merge both of your PRs into our trunk and miss that there would have been broken tests because of the PRs conflicting / depending on one another)

AliSoftware · 2022-11-07T12:29:25Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/github_helper.rb

+      #  if `true`, will subtract 3 days from the `:number_of_days_from_code_freeze_to_release`.
+      #  if `false`, will use the days of `:newmilestone_duration`
+      #
+      def create_milestone(repository:, newmilestone_number:, newmilestone_duedate:, newmilestone_duration:, number_of_days_from_code_freeze_to_release:, need_submission:)


The parameter names seems a bit of a mouthful to me, given we already know from the context (of them being parameters of create_milestone) that those are about new milestones.

For example, what about naming the parameters just title, due_date, duration, and days_until_release?

Though a probably even nicer solution, especially because duration and need_submission are only used once and are kind of exclusive (i.e. either we pass need_submission: false and then duration doesn't matter, or we pass need_submission: true and that's only when duration is used…), we should probably combine the goal of the two parameters into a single one there, which would be days_until_submission, and let the caller pass to this value.

Given that, I suggest the following new API for this helper method:

def create_milestone(repository:, title:, due_date:, days_until_submission:, days_until_release:)

Then:

For the time being, the call site in create_new_milestone_action would just do the computation on its end, i.e. pass params[:need_appstore_submission] ? number_of_days_from_code_freeze_to_release - 3 : milestone_duration for the new days_until_submission parameter of this GithubHelper#create_milestone, carrying over the "Using 3 days mostly for historical reasons" logic from lines 88-89 for now

In the future, when we will consider updating the API of the create_new_milestone action itself one day, we would then be able to update its ConfigItem (in a breaking-change way) to have the callers also directly provide the days_until_submission and days_until_release values in their Fastfile's call sites instead of duration and/or need_submission

But for now, that change would make for a nicer and easier to read and understand API for the internal GithubHelper#create_milestone, all while preparing for the future of the planned refactor of the corresponding action and public API.

For sure, those parameters that are in place are big!
I'll address those changes, thanks for the input 😄

raafaelima · 2022-11-07T12:48:10Z

I was a bit surprised that this PR didn't include the necessary changes to create_new_milestone_action_spec.rb on those lines.

Then I realized it was because that PR was targeting trunk (as opposed to being built on top of your raafaelima:add/unit-tests-for-milestone-actions branch from #425… which I guess you didn't do because you wouldn't have been able to open a PR in our repo otherwise, but instead only be able to open that PR on your fork?).

Yep, Is the same case that we had about merging the #420 first, and then opening the unit tests because I can't stack PRs out of the trunk, because I'm on a fork, and my branch did not exists on the main repo, to stack other branchs on top of it.

Would have been nice if you had mentioned that limitation / aspect in the description of the PR — or alternatively kept the PR as Draft (and commented on why) while you were waiting for #425 to land before un-drafting it — so that we wouldn't be surprised. Especially given that regardless if we merge #425 or this #426 first, you will have to adjust the tests in other one before we land it anyway (and as CI doesn't run on fork'd PRs by default for security reasons, it would be easy to accidentally merge both of your PRs into our trunk and miss that there would have been broken tests because of the PRs conflicting / depending on one another)

About that, I have made the dependency commend at #425 description but forget to add it here to double-cross the dependency information to review. My Bad! Already fixing it. Thanks! 😄

raafaelima · 2022-11-07T14:39:17Z

Just throwing some thoughts 🤔 💭

That limitation of mine about not being able to open stacked PRs is leading to some confusion and, I'm not sure what is the best path here tbh 😵‍💫

If it is to wait for PR#425 to be merged and then update PR#426, to fix the unit tests that will break with the change of parameters. So, therefore this should be a draft PR until the merge of PR#425 happens.
if we should do the opposite (as was my initial thought), and review and merge this PR#426 first, then go to PR#425, and fix the unit tests at create_new_release_action that will fail because of PR#426 changes.

As I can't go with the ideal scenario (the stacked PRs) I really do not have a strong option on this,
I'd like to hear your thoughts on it @AliSoftware 😄

AliSoftware · 2022-11-07T16:07:22Z

About that, I have made the #425 (comment) but forget to add it here to double-cross the dependency information to review. My Bad! Already fixing it. Thanks! 😄

I totally missed that comment, very likely because you added it after the fact and I didn't re-read the whole description on my subsequent reviews of #425 after I read it on the first pass 😅 This is why I didn't realize your plan was to land 426 first, and amend 425 afterwards (which is fine), rather than the other way around as I initially thought.

In any case, it's always a good habit to put the information in both, because anyone who might review your PR #426 might not be aware of / not have read or seen PR #425.

AliSoftware · 2022-11-07T16:10:19Z

That limitation of mine about not being able to open stacked PRs is leading to some confusion and, I'm not sure what is the best path here tbh 😵‍💫

Yeah, it's not an ideal situation due to you working on a fork, I acknowledge that!

Given your plan was, as I now see you mentioned by updating your description of #425 and given the limitation of the fork, to merge this #426 one first, and then rebase and update #425 accordingly, I think it's ok to go with that then. I added the "Do Not Merge" label (that we use for such occasion) to #425 to indicate that it's pending this #426 to land first then.

mokagio · 2022-11-07T16:36:27Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_new_milestone_action.rb

+        # If there is a review process, we want to submit the binary 3 days before its release
+        # Using 3 days is mostly for historical reasons where we release the apps on Monday and submit them on Friday.


What do you think of this rewording?

Suggested change

# If there is a review process, we want to submit the binary 3 days before its release

# Using 3 days is mostly for historical reasons where we release the apps on Monday and submit them on Friday.

# Because of the app stores review process, we submit the binary 3 days before the intended release date.

# Using 3 days is mostly for historical reasons, for a long time, we've been submitting apps on Friday and releasing them on Monday.

AliSoftware · 2022-11-07T16:18:11Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/github_helper.rb

+      # @param [Time] due_date milestone due date (e.g. `2022-10-22T12:00:00Z`)
+      # @param [Integer] days_until_submission Number of days until submission


Suggested change

# @param [Time] due_date milestone due date (e.g. `2022-10-22T12:00:00Z`)

# @param [Integer] days_until_submission Number of days until submission

# @param [Time] due_date milestone due date—which will also correspond to the code freeze date

# @param [Integer] days_until_submission Number of days from code freeze to submission to the App Store / Play Store

AliSoftware · 2022-11-07T17:35:58Z

spec/github_helper_spec.rb

@@ -234,14 +234,14 @@ def get_milestone(milestone_name:)
    end

    def create_milestone(need_submission:, milestone_duration:, days_code_freeze:)
+      days_until_submission = need_submission ? (days_code_freeze - 3) : milestone_duration


Since this file is focused on testing the helper, and the helper doesn't have knowledge of the concept of need_submission anymore, I think we should adapt the github_helper_spec accordingly, to write the it tests based on the new parameters (i.e. don't test "has correct dates (with|without) submission" cases, but test things like "computes the correct dates for (a standard case | dates crossing DST boundaries | inconsistent offsets | …)".

Also begs the question what the behavior should do if we provide inconsistent inputs (invalid date, negative values for days_until_*, days_until_release < days_until_submission, …?).
Maybe it's ok to let that to the caller's responsibility and not check (i.e. keep the current behavior), at least for now; but adding tests to the action like here would also be the best time to raise this kind of questions (after all, if we try to follow in the footsteps of TDD, we should write the tests to describe the behaviors and use cases we want first, and then we implement them to match, not the other way around).

Also begs the question what the behavior should do if we provide inconsistent inputs (invalid date, negative values for days_until_*, days_until_release < days_until_submission, …?). [.....]

That's a question that I keep asking myself 🤔 💭

Using as an example a case where I have more days from submission than from release (i.e days_until_submission: 14, days_until_release: 3), The submission date will be waaay after the release date. In this case, should we raise a FastlaneError?

# Milestone with due_date = '2022-10-20', days_until_submission = 14 and days_until_release = 3 Code freeze: October 20, 2022 App Store submission: November 03, 2022 Release: October 23, 2022

Also, in the cases that we have negative values for these same parameters (i.e days_until_submission: -14, days_until_release: -14), the Ruby DateApi at param.to_datetime.next_day(..) just subtracts days from the due_date, although is allowed, it is another inconsistency. So, negative values or zero should be allowed on these parameters, in the first place?

# Milestone with due_date = '2022-10-20', days_until_submission = -14 and days_until_release = -14 Code freeze: October 20, 2022 App Store submission: October 06, 2022 Release: October 06, 2022

Also, assuming that when we refer to Release, we are talking about the date when this version will be available to our users, and by AppStore Submission, is the date we submit the code to be reviewed and alpha/beta tested.

What is the expected behavior of the dates when creating milestones?

The Code Freeze date should always be before AppStore Submission and Release dates?

The Code Freeze date is always before AppStore Submission, or those can also be equal?

The Release date is always after AppStore Submission, or those can also be equal?

Should this be validated at all?

Validating dates is always tricky, I just feel that I should confirm my assumptions before starting anything 😅

I also feel that I should write a mini-P2 to document those changes, just in case anyone passes through here in the future and needs to do any refactorings. 🗒️

Yep writing a P2 seems a good idea.

As for the cases you mention above, those seems to be good ones; I think we should add those, then use UI.user_error! in the implementation for when those cases are encountered.

What is the expected behavior of the dates when creating milestones?

Every sprint we:

Do the code freeze for version N on Monday, which corresponds to us cutting a release/* branch from trunk, adding ❄️ to the N milestone, pushing some new adjustments commits in the release/* (like freezing the strings to send to translation, bumping the version numbers, …), then create a first beta that we send to beta testers

Then we let that period of beta test on the code-frozen beta run for almost a whole sprint (while developers are working on trunk on version N+1 in parallel), doing new betas from the release/* branch during that period if there are last-minute betafixes to land

On the Friday at the end of the Sprint, once the beta-test period is over and we hopefully beta-fixed anything that was urgent to fix during that period, we submit the final build to the App Store / Play Store

On the next Monday, providing that Apple/Google have approved the build we submit over the weekend, we start the rollout of that build to end users, and start a new sprint (code freeze of version N+1; wash, rince, repeat)

This process is also described in this old blog post, though it might be a tad outdated now.

Nowadays some of our apps still have 2-weeks sprints, which means when we do a code freeze on Day D+0 (Monday, start of sprint), we then submit for review on Day D+11 (i.e. second Friday), hope it gets approved over the weekend (it usually is), and release on Day D+14 (next Monday), same day we start a new code freeze. Some other apps use 1-week sprints, which means code freeze at D+0, submission at D+4, release at D+7.

…descriptive

…order

AliSoftware · 2022-11-09T19:34:47Z

@raafaelima FYI, the CI failure is due to the following failure when running the tests (bundle exec rspec):

1) FastlaneCore::Helper::GithubHelper create_milestone computes the correct dates when the due date is on the verge of a DST day change
  | Failure/Error: expect(client).to receive(:create_milestone).with(test_repo, test_milestone_number, options)
  |  
  | #<InstanceDouble(Octokit::Client) (anonymous)> received :create_milestone with unexpected arguments
  | expected: ("repo-test/project-test", "10.0", {:description=>"Code freeze: June 19, 2022\nApp Store submission: June 21, 2022\nRelease: June 22, 2022\n", :due_on=>"2022-06-19T12:00:00Z"})
  | got: ("repo-test/project-test", "10.0", {:description=>"Code freeze: June 18, 2022\nApp Store submission: June 20, 2022\nRelease: June 21, 2022\n", :due_on=>"2022-06-18T12:00:00Z"})
  | Diff:
  | @@ -1,6 +1,6 @@
  | ["repo-test/project-test",
  | "10.0",
  | {:description=>
  | -   "Code freeze: June 19, 2022\nApp Store submission: June 21, 2022\nRelease: June 22, 2022\n",
  | -  :due_on=>"2022-06-19T12:00:00Z"}]
  | +   "Code freeze: June 18, 2022\nApp Store submission: June 20, 2022\nRelease: June 21, 2022\n",
  | +  :due_on=>"2022-06-18T12:00:00Z"}]

AliSoftware · 2022-11-09T19:39:34Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/github_helper.rb

+        UI.user_error!('days_until_release must be greater than zero.') if days_until_release <= 0
+        UI.user_error!('days_until_submission must be greater than zero.') if days_until_submission <= 0
+        UI.user_error!('days_until_release must be greather than days_until_submission') if days_until_submission >= days_until_release


It feels to me that this would be more readable if the conditions matched the messages described in the user_error!, rather than being the negative of those ("must be greater than zero" --> use a days_until-release > 0 condition), otherwise it needs your brain to think what the negative should be to ensure that the if is correct and matching the message.

In fact, for the 3rd case I think the condition and messages are incorrect, because it could be valid to consider that the submission and the release could be on the same day (that's not a common/used setup for any of our apps currently, but might be one day, at least it shouldn't be invalid imho)

Suggested change

UI.user_error!('days_until_release must be greater than zero.') if days_until_release <= 0

UI.user_error!('days_until_submission must be greater than zero.') if days_until_submission <= 0

UI.user_error!('days_until_release must be greather than days_until_submission') if days_until_submission >= days_until_release

UI.user_error!('days_until_release must be greater than zero.') unless days_until_release > 0

UI.user_error!('days_until_submission must be greater than zero.') unless days_until_submission > 0

UI.user_error!('days_until_release must be greater or equal to days_until_submission') unless days_until_release >= days_until_submission

Thanks for the feedback, I was not aware of the unless keyword, indeed is more legible that way. 😄

AliSoftware · 2022-11-09T19:41:47Z

spec/github_helper_spec.rb

+    it 'computes the correct dates when the due date is on the verge of a DST day change' do
+      # The PST to PDT (DST) change is made on the second Sunday in March and finishes on the first Sunday in November
+      Time.zone = 'Pacific Time (US & Canada)'
+      due_date = '2022-06-18T23:00:00Z'.to_time


# The PST to PDT (DST) change is made on the second Sunday in March and finishes on the first Sunday in November

Then that means that the date you used (2022-06-18 is not even close to a DST change then, since it's neither close to March nor November… so this actually tests the opposite case?

I wanted to check the PST to PDT details and landed on Wikipedia. Here's the link for future reference, just in case.

I had problems testing this properly, to be honest. It confuses me that the fact that PST/PDT changes are made at 2 am, Also for some reason, the date to not change when I use the 14 march (the actual DST change date) as my date, even forcing the timezone to be at PST. So I just used any date inside the DST changes, given the date that was put was on UTC.

Due to the Ci failure, I think that this is not the proper way to enforce the use of a specific timezone (although it passes here on my local test run). I'll try to build it differently, I was researching other ways to do this and found this one, which is wrapping the test on the timezone change block:

Time.use_zone('Europe/London') do due_date = Time.zone.parse('2022-03-27 23:00:00') # ... end

TBH, I'm not sure if this is the correct way to do it either, as there are a lot of different answers on how to do this (most of them on rails), on StackOverflow. If you folks have suggestions on how to deal with this, I would love to hear @AliSoftware @mokagio 😄

Note: I'll use the DST from Europe this time, that makes more sense in my head than the American ones. 😅

From the top of my head I don't remember what's the right way to do it, though Time.use_zoen(…) do…end feels like a good lead. In any case, I'd recommend picking a date that you know is before both Europe and Pacific DST change dates, and use an offset for the date addition that is large enough to end up after both DST change dates as well. E.g. a date at start of Oct and an offset of 60 days, or something like that, to be sure to cover both cases anyway, even if Time.use_zone should do the trick.

AliSoftware · 2022-11-09T19:46:49Z

spec/github_helper_spec.rb

+    it 'computes the correct dates for standard period' do
+      due_date = '2022-10-20T08:00:00Z'.to_time.utc


This example date feels dangerously close to the actual DST change date zone—e.g. in Europe, DST starts on the last Sunday of March and ends on the last Sunday of October, so it happened on Oct 30 this year. Which is still outside the Oct 20–Oct 27 date range that is used in this example, but still very close.

AliSoftware · 2022-11-09T19:48:09Z

spec/github_helper_spec.rb

+      it 'raises an error if submission date is equal to release date' do
+        due_date = '2022-10-20T08:00:00Z'.to_time.utc
+        expect { create_milestone(due_date: due_date, days_until_submission: 1, days_until_release: 1) }
+          .to raise_error(FastlaneCore::Interface::FastlaneError, 'days_until_release must be greather than days_until_submission')
+      end


This should be a valid case imho (see other comment)

Yeah, I was a bit confused if this should be or not be a valid case, Initially, I think it was, but as you explained here about the process, it feels that the submissions always need to be before release, and not fall in the same day. I'll change that test to be a valid case, thanks for the input. 😄

mokagio · 2022-11-10T09:56:20Z

spec/github_helper_spec.rb

+      create_milestone(due_date: due_date, days_until_submission: 2, days_until_release: 3)
+    end
+
+    context 'with input validation' do


Out curiosity, why did you choose to use a context here?

I thought that will be more organized and improve the readability to have the input validations under the context, instead of having them grouped under the main describe block.

Although, now that you ask that question, it made me think if this is really useful at all 🤔
As I do not have any additional behavior to input on those methods, the context here was just misused. 😄

I think Gio's question was more about why using context vs describe to group the tests. Functionally that doesn't make a difference, but semantically, context is more for when [in situation X] or with [given use case or situation]

@raafaelima @AliSoftware, a bit of both 😄

Why grouping the input validation? (You answered that already) And why with context vs describe?

It is my understanding that context is an alias for describe but I can't find the code where that happens. The closer I could get to was the DSL source code which is very meta, and the documentation which implies they are.

In particular, the example from the docs use context with a "when ..." like Olivier suggested above.

RSpec.describe "something" do context "when something is a certain way" do it "does something" do # example code goes here end end end

That pattern matches my experience with and mental model of describe vs context and it's why seeing this code tickled my interest and made me ask the question 😄

Some additional doc / reference about context vs describe:

https://rspec.rubystyle.guide/#example-group-naming

https://rspec.rubystyle.guide/#context-descriptions

https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspeccontextmethod

AliSoftware · 2022-11-16T19:14:11Z

Note that Rubocop is reporting the two following codestyle offenses:

$ bundle exec rubocop
Inspecting 164 files
.....................................................................................................C..............................................................

Offenses:

lib/fastlane/plugin/wpmreleasetoolkit/helper/github_helper.rb:83:80: C: [Correctable] Style/NumericPredicate: Use days_until_release.positive? instead of days_until_release > 0.
        UI.user_error!('days_until_release must be greater than zero.') unless days_until_release > 0
                                                                               ^^^^^^^^^^^^^^^^^^^^^^
lib/fastlane/plugin/wpmreleasetoolkit/helper/github_helper.rb:84:83: C: [Correctable] Style/NumericPredicate: Use days_until_submission.positive? instead of days_until_submission > 0.
        UI.user_error!('days_until_submission must be greater than zero.') unless days_until_submission > 0
                                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^

raafaelima · 2022-11-16T19:26:10Z

Thanks for the catch, fixed on the last commit

AliSoftware

Looking good
Thanks for adding so many test cases around timezone boundaries 👍

AliSoftware · 2022-11-16T19:55:41Z

spec/github_helper_spec.rb

+        due_date = Time.zone.parse('2022-03-27 23:00:00Z')
+        options = {
+          due_on: '2022-03-28T12:00:00Z',


Nice edge case of having 2022-03-27 23:00:00Z being exactly 2022-03-28 00:00:00 +0100 because that day is the exact day of the DST change, when London has switched to +0100… while for your next test case on line 259, the 2022-03-26 23:00:00Z date still corresponds to 2022-03-26 23:00:00 +0000 because this is just before the DST change when London were still on +0000…

Took me a while to figure that out though and to triple-check that this difference between the due_date (2022-03-27) and the due_on (2022-03-28) was the correct and expected behavior here, and not an incorrect value due to our date math being buggy or whatnot. Phew!

Could maybe benefit from some small code comment (here and on line 259) to give more context about those tricky details?

For sure! 😄

Besides the comments, I also added another test to cover the scenario of the due_date is not close enough to the end of the day to exist a date change (Line 259) and also add a new case, to test what happens if you have a date close to the day change, but not in DST (Line 278).

… spec to cover an addional case

AliSoftware · 2022-11-16T21:22:24Z

spec/github_helper_spec.rb

+        # March 27th, 2022 is the exact day that London switches to the DST (+1h)
+        # If the due date is too close to the next day, a day change will happen
+        # So, 2022-03-27 23:00:00Z will be exactly 2022-03-28 00:00:00 +0100 at the DST change


mokagio · 2022-11-17T16:59:07Z

Thanks for adding so many test cases around timezone boundaries 👍

+1 – Thanks @raafaelima

raafaelima added 3 commits November 6, 2022 16:23

GithubHelperSpec: Add Yard Docs and Named Parameters to create_milest…

66ccfcf

…one method To become this method cleaner and more legible to call, we're adding named parameters to it, following the name of the current ones, and also adding yard docs to help guide the new people who use it

CreateNewMilestoneAction: Adds named parameters to create_milestone G…

b658f87

…ithubHelper's method

GithubHelper#create_milestone: Rephrase yard docs

10528de

raafaelima mentioned this pull request Nov 6, 2022

Add Unit tests for milestone actions #425

Open

AliSoftware requested review from mokagio and AliSoftware November 7, 2022 11:55

AliSoftware reviewed Nov 7, 2022

View reviewed changes

GithubHelper#create_milestone: Rename parameters to be more descriptive

255d108

mokagio reviewed Nov 7, 2022

View reviewed changes

AliSoftware reviewed Nov 7, 2022

View reviewed changes

raafaelima requested review from AliSoftware and mokagio and removed request for AliSoftware and mokagio November 8, 2022 15:05

raafaelima added 2 commits November 8, 2022 20:50

CreateNewMilestoneAction: Rephrase comment about release times to be …

99a95ac

…descriptive

GithubHelper#create_milestone: Add unit tests and validation to date …

39b23c8

…order

raafaelima requested review from AliSoftware and removed request for mokagio November 8, 2022 20:53

AliSoftware reviewed Nov 9, 2022

View reviewed changes

mokagio reviewed Nov 10, 2022

View reviewed changes

raafaelima requested review from AliSoftware and removed request for AliSoftware November 10, 2022 11:46

raafaelima requested review from mokagio and AliSoftware and removed request for mokagio November 10, 2022 11:46

raafaelima added 3 commits November 14, 2022 08:06

Merge branch 'trunk' into add/named-params-create-milestone

0750081

GithubHelper: Fix Timezone changes unit tests

b218ccc

GithubHelper: Adds more unit tests to validate the Timezone changes

de90d4d

raafaelima requested review from mokagio and AliSoftware and removed request for AliSoftware and mokagio November 15, 2022 16:33

GithubHelper: Fix Rubocop Issues

d87c03c

AliSoftware approved these changes Nov 16, 2022

View reviewed changes

GithubHelperSpec: Adds comments to DST cases to be more clear and new…

7f99d51

… spec to cover an addional case

AliSoftware reviewed Nov 16, 2022

View reviewed changes

AliSoftware merged commit df29661 into wordpress-mobile:trunk Nov 17, 2022

		# If there is a review process, we want to submit the binary 3 days before its release
		# Using 3 days is mostly for historical reasons where we release the apps on Monday and submit them on Friday.

		# @param [Time] due_date milestone due date (e.g. `2022-10-22T12:00:00Z`)
		# @param [Integer] days_until_submission Number of days until submission

		it 'computes the correct dates for standard period' do
		due_date = '2022-10-20T08:00:00Z'.to_time.utc

Add named params and yard docs to create_milestone method at GithubHelper #426

Add named params and yard docs to create_milestone method at GithubHelper #426

Conversation

raafaelima commented Nov 6, 2022 • edited Loading

What does this solve?

How do the yard docs look now?

Dependency

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafaelima commented Nov 7, 2022 • edited Loading

raafaelima commented Nov 7, 2022 • edited Loading

AliSoftware commented Nov 7, 2022

AliSoftware commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafaelima Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

AliSoftware commented Nov 9, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware commented Nov 16, 2022

raafaelima commented Nov 16, 2022

AliSoftware left a comment

Choose a reason for hiding this comment

AliSoftware Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Nov 17, 2022

Add named params and yard docs to `create_milestone` method at `GithubHelper` #426

Add named params and yard docs to `create_milestone` method at `GithubHelper` #426

raafaelima commented Nov 6, 2022 •

edited

Loading

raafaelima commented Nov 7, 2022 •

edited

Loading

raafaelima commented Nov 7, 2022 •

edited

Loading

raafaelima Nov 7, 2022 •

edited

Loading

AliSoftware Nov 8, 2022 •

edited

Loading

AliSoftware Nov 16, 2022 •

edited

Loading