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.

Sign up for GitHub

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

Jump to bottom

Finally solved issue #1734 #1766

Merged
merged 4 commits into from
Mar 5, 2024
Merged

Finally solved issue #1734 #1766

merged 4 commits into from
Mar 5, 2024

Conversation

quadcopterman
Copy link
Contributor

No description provided.

@quadcopterman quadcopterman self-assigned this Feb 29, 2024
@CLAassistant
Copy link

CLAassistant commented Feb 29, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions GitHub Actions
Copy link

Pull reviewers stats

Stats of the last 30 days for popstellar:

User Total reviews Time to review Total comments
pierluca
🥇		 5
▀▀▀		 2d 6h 18m
▀▀		 8
▀▀▀▀
MariemBaccari
🥈		 5
▀▀▀		 1d 17h 15m
▀		 1
▀
sgueissa
🥉		 2
▀		 4d 21h 28m
▀▀▀▀		 8
▀▀▀▀
jbsv
 2
▀		 1d 7h 33m
▀		 0
arnauds5
 1
▀		 6h 55m
 0
osm-alt
 1
▀		 5h 52m
 0
ljankoschek
 1
▀		 20h 9m
▀		 0
K1li4nL
 1
▀		 18m
 0
emonnin-epfl
 1
▀		 6h 45m
 1
▀

@Kaz-ookid Kaz-ookid marked this pull request as ready for review February 29, 2024 19:20
@Kaz-ookid Kaz-ookid requested a review from a team as a code owner February 29, 2024 19:20
@Kaz-ookid Kaz-ookid marked this pull request as draft February 29, 2024 19:43
@Kaz-ookid
Copy link
Contributor

my bad for marking it as ready to review, forgot some tests are to come

@quadcopterman quadcopterman linked an issue Mar 4, 2024 that may be closed by this pull request
Failing test under specific time conditions on FE2 #1734
Closed
@quadcopterman
Copy link
Contributor Author

This PR is another way to solve issue #1734 (much much simpler than my previous attempt) : just let the day comparison go to -1 when comparing the start date of an event (in case midnight is just between start date and now). If this event is too far in the past, it will be handeled by computeTimeInSeconds() as Instant also contains the date.
A test was added to make sure that a 1 day old date won't work and that this issue won't occur again

@quadcopterman quadcopterman marked this pull request as ready for review March 4, 2024 18:18
matteosz
matteosz reviewed Mar 4, 2024
View reviewed changes
Copy link
Contributor

@matteosz matteosz left a comment

Choose a reason for hiding this comment

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

LGTM!

Just 1 small comment

.../app/src/main/java/com/github/dedis/popstellar/ui/lao/event/AbstractEventCreationFragment.kt Outdated
if (compareWithNowByDay(newDate) < 0) {
// let the comparison go to -1 for cases where the time is just after midnight
// this is handled just fine by computeTimeInSeconds() as an Instant also contains the date
if (compareWithNowByDay(newDate) < -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor opportunity: this check appears twice (here and line 224), so it'd be better to extract it into a variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for spotting that, I'll correct it

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 4, 2024

Quality Gate Passed Quality Gate passed for 'PoP - PoPCHA-Web-Client'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 4, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Be1-Go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 4, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Be2-Scala'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 4, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Fe2-Android'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 4, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Fe1-Web'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@matteosz matteosz self-requested a review March 5, 2024 09:25
matteosz
matteosz approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@matteosz matteosz left a comment

Choose a reason for hiding this comment

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

LGTM!

@quadcopterman quadcopterman merged commit d372da6 into master Mar 5, 2024
18 checks passed
@quadcopterman quadcopterman deleted the work-fe2-johan branch March 5, 2024 12:25
@quadcopterman quadcopterman linked an issue Mar 11, 2024 that may be closed by this pull request
Highlight our token during Roll Calls #1776
Closed
@quadcopterman quadcopterman removed a link to an issue Mar 11, 2024
Highlight our token during Roll Calls #1776
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kaz-ookid Kaz-ookid Kaz-ookid approved these changes

@matteosz matteosz matteosz approved these changes

Assignees

@quadcopterman quadcopterman

Labels
fe2-android
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failing test under specific time conditions on FE2
4 participants
@quadcopterman @CLAassistant @Kaz-ookid @matteosz