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

Change to ArrayField for TimeRecurrence.days from MultiSelectField #711

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Change to ArrayField for TimeRecurrence.days from MultiSelectField #711

merged 3 commits into from
Feb 12, 2024

Conversation

johannaengland
Copy link
Contributor

@johannaengland johannaengland commented Jan 31, 2024 •
edited
Loading

First part for solving #707.

This was easier than expected, which makes me suspicious...But tested against current frontend and all looks good.

@johannaengland johannaengland added the data model Affects the data model and/or SQL schema label Jan 31, 2024
@johannaengland johannaengland self-assigned this Jan 31, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 31, 2024 •
edited
Loading

Test results

       8 files     584 suites   16m 43s ⏱️
   401 tests    400 ✔️ 1 💤 0 ❌
3 208 runs  3 200 ✔️ 8 💤 0 ❌

Results for commit e816de6.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (74e7d83) 79.40% compared to head (68dc2b2) 79.44%.
Report is 5 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
+ Coverage   79.40%   79.44%   +0.04%     
==========================================
  Files          73       73              
  Lines        3539     3546       +7     
==========================================
+ Hits         2810     2817       +7     
  Misses        729      729

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johannaengland johannaengland requested a review from hmpf February 9, 2024 11:08
hmpf
hmpf reviewed Feb 9, 2024
View reviewed changes
Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

I will do a manual test, then approve.

@hmpf hmpf mentioned this pull request Feb 12, 2024
Merged
@hmpf
Copy link
Contributor

hmpf commented Feb 12, 2024

You're still importing MultiSelectField in src/argus/notificationprofile/models.py, and we cannot remove it from pyproject.toml until the release after the next.

@hmpf hmpf self-requested a review February 12, 2024 10:02
hmpf
hmpf reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

Manual test passed but see comment.

I'm still surprised you went with an ArrayField and not a lookup table. (I do have SQL brain though.)

@johannaengland
Copy link
Contributor Author

I'm still surprised you went with an ArrayField and not a lookup table. (I do have SQL brain though.)

I can still change that if you think a lookup table is the better solution?

@johannaengland johannaengland requested a review from hmpf February 12, 2024 10:18
@hmpf hmpf added the dependencies Run `tox -r` before testing locally, dependencies have changed label Feb 12, 2024
@hmpf
Copy link
Contributor

hmpf commented Feb 12, 2024

All good!

@johannaengland johannaengland force-pushed the remove-multi-select-field branch from a429113 to e816de6 Compare February 12, 2024 11:14
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@johannaengland johannaengland merged commit aae5538 into Uninett:master Feb 12, 2024
10 checks passed
@johannaengland johannaengland deleted the remove-multi-select-field branch February 12, 2024 11:25
@johannaengland johannaengland mentioned this pull request Apr 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmpf hmpf hmpf approved these changes

@lunkwill42 lunkwill42 Awaiting requested review from lunkwill42

@stveit stveit Awaiting requested review from stveit

Assignees

@johannaengland johannaengland

Labels
data model Affects the data model and/or SQL schema dependencies Run `tox -r` before testing locally, dependencies have changed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johannaengland @codecov-commenter @hmpf