Track timezones by name, if possible (Fix #4221) #7399
Conversation
npracht
added
bug
npracht
moved this from Changes Requested / Review
to Ready to Test (first time)
in Mautic 2
|
Wasn't double tested before the 2.15.2 PR merge deadline so pushing to the next release. |
npracht
added
Triage M2/M3
and removed
code-review-needed
|
Hi there! We now want to integrate your contribution in the Mautic 3 roadmap as 3.0.1 candidate. How to do?
Please report results by commenting on your PR to make us administration easier. In case your bugfix only apply to Mautic 2, we'll consider adding it in an extra Mautic 2 version. You can more information on how to do all of that on this blog post "Getting you PR ready for Mautic 3". |
|
Hello @pjeby could you please test mautic 3.0.0-beta2 and tell me if the issue is still existing?
Waiting for your feedback, i would very love to merge it in 3.0.1 if still relevant. Thanks ! |
|
Hi @pjeby would you kindly have a look at conflicts ? |
npracht
added
the
has-conflicts
|
@pjeby as we haven't heard back on this in the past 6 months and there's an outstanding conflict I'm going to close the PR. If someone would like to pick this up in the future, please create a new PR and reference this one in the description. |
|
@RCheesley The last two months I've been waiting for a reply from you on #4221, which is the bug this PR fixes. You closed the bug and did not reply to my inquiry as to what fingerprinting has to do with timezone detection. See the discussion here |
|
I vote for reopen this PR. It's not related to fingerprint and fix really ugly issue. @pjeby Are you able to resolve conflicts? |
|
@pjeby the PR was closed due to a lack of activity despite several requests since April. If you're willing to work on this and resolve the conflicts then we can consider it for inclusion in a future release. Apologies for my misunderstanding on the issue, we were triaging over 500 issues and once or twice I mis-read the threads and made an incorrect assumption. |
|
I've just taken a look at the conflicts, and it is not at all clear to me how to proceed. I think you would need to have the person who stripped out the fingerprinting take a look, if you want something done quickly. It will take me a longer time to have the availability to dive into the new code base, since I am now running a private fork due to the extreme delays of getting bug fixes or even the most minor of features merged here. Speaking of delays, please note that the only action the Mautic team took on this PR for nearly one full year (April 18, 2019 to April 4, 2020) was to tag and retag it and bump it from one milestone to another, when at any time it could've been merged without conflicts. In the meantime, I had a business to run and had to essentially fork Mautic to incorporate a fair number of PRs, including my own, in order to get basic advertised functionality (e.g. timezone detection) to work correctly. You can probably understand why I would be less than thrilled with the PR being closed for "inactivity" that is nearly three times shorter than the Mautic team's inactivity on the same issue, and also the direct result of that inactivity... and which only needs additional work now because it wasn't merged at any time during the preceding twelve months of inactivity. |
|
Hi @pjeby and thanks for getting back to us. We really do appreciate how frustrating it is to make a contribution and have it sit waiting for so long without action being taken. You are not alone in this - we had over 300 open PR's and over 900 open issues to process which had accumulated over years. It sucks that it got this bad, but we are squarely focused on moving forward rather than dwelling on the past. As you may have noticed we now have an established governance model after extensive community consultation, teams and working groups are coming to life in the community, and we are making a lot of progress in dealing with the vast amount of technical debt that had accumulated. There are a few cases where we have closed issues and PR's due to inactivity and then re-opened them when the authors have re-emerged. On the whole, most of the ones that have been closed are genuinely inactive. We have to draw the line somewhere, and we felt that we were fair in doing so, giving quite a lot of time for folks to respond before we closed anything down. It has not been easy with such a small team who are almost exclusively (other than myself) doing this in their spare time alongside full time work and caring responsibilities. To be clear, this repo is managed by the community, so 'the Mautic Team' you refer to are the product team and release leads, who I can count on one hand. The team was formed in November 2019 after our Community Sprint in Amsterdam where we started to establish the structures from the governance model. Also, each PR requires testing before it is merged, so we also rely on folk helping us to test the PR's so we can get them into a release. It may be worth thinking about supporting that initiative (testing of PR's) as a business that depends on Mautic, as that is a direct way in which you can help us to get more fixes and features into Mautic and an area in which we are very short of contributors. We have a sprint on the Friday and Saturday before each release freeze which is an ideal time to block out some time if you are able. The next sprints are 16-17 October, following the MautiCon event (17th November), and 18-19 December. Our biggest factor slowing us down right now is the tiny number of folk who are helping to test, supporting the release process, and generally keeping things moving forward. A huge amount of work is falling on very few people and we are desperately trying to keep things moving. So, we totally appreciate the frustration and understand if you do not have the time to address the conflicts, but we simply cannot do anything with your PR until they are addressed. We value any time that folk can contribute to help us get these remaining PR's over the line, or to help in any way with keeping us move forward. Regarding the fingerprinting being removed, here is the PR that removed the feature: #8428 in case that helps at all. @escopecz may be able to give some guidance as he worked on that PR. Feel free to reach out to me directly on Slack or by email at ruth.cheesley@acquia.com if you would like to discuss any of the issue above further. Thanks again for being willing to take a look at the PR - hopefully we will be able to merge it in a future release! |
|
I would suggest to follow the similar path we did with the I hope it helps. |
|
Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact @RCheesley. CLA has not been signed by @pjeby. |
Codecov Report
@@ Coverage Diff @@
## features #7399 +/- ##
===========================================
Coverage 39.64% 39.64%
- Complexity 33978 33984 +6
===========================================
Files 1989 1989
Lines 105730 105739 +9
===========================================
+ Hits 41913 41917 +4
- Misses 63817 63822 +5
|
|
@pjeby can you please sign the contributors agreement linked in the post above so we can look at getting this tested and merged? |
|
Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @pjeby. |
|
Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @pjeby. |
|
Is it doing the same thing as #8205 ? |
|
After careful review of the CLA, I am going to have to decline. To be clear, I don't have any issue with doing this type of IP assignment for my pull requests. The issue is that it states that I am assigning the rights in "any original work" I create that is "submitted to us through any manner of communication", which covers an awful lot of things besides pull requests, and seems it could easily be construed to cover almost any IP I put out in the public, or if somebody in your company happens to subscribe to my email list. So, if at some point there is a more narrowly defined CLA that covers the specific methods of submission, I'd be willing to review it again. But as written, it basically sounds like a blanket grant of any IP I don't specifically keep secret or explicitly disclaim submission of, which is not compatible with my own business. |
|
Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @pjeby. |
closes #4221
Please be sure you are submitting this against the staging branch.
Description:
Sufficiently-modern browsers support detecting the actual timezone name, not just the offset. This PR checks for this support, and if available, uses it in place of the timezone offset to determine a lead's preferred timezone, thereby fixing #4221.
In the event that the browser doesn't support this feature, a secondary approach is used, based off the algorithm and table used by jstimezonedetect to determine a timezone, taking into account the existence of daylight savings or "summer" time and the direction of change. This approach still has some ambiguity, but only adds a few lines of javascript, vs. including the full 12k of jstimezonedetect, and has much better accuracy than the offset-only method that Mautic used previously. (But if no match is found using the new algorithm, the offset-only method is used as a fallback.)
Steps to reproduce the bug:
Steps to test this PR:
List deprecations along with the new alternative:
List backwards compatibility breaks: