feat: add watchers field to the Course Edit Form #890
Conversation
AfaqShuaib09
force-pushed
the
afaq/prod_3412
branch
2 times, most recently
from
aef2a67
to
4a66f31
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #890 +/- ##
==========================================
+ Coverage 66.86% 66.95% +0.09%
==========================================
Files 128 128
Lines 3190 3199 +9
Branches 925 930 +5
==========================================
+ Hits 2133 2142 +9
Misses 1008 1008
Partials 49 49
☔ View full report in Codecov by Sentry. |
AfaqShuaib09
force-pushed
the
afaq/prod_3412
branch
from
4a66f31
to
7c909ba
Compare
AfaqShuaib09
changed the title
AfaqShuaib09
force-pushed
the
afaq/prod_3412
branch
from
d26d5e8
to
d87fe8f
Compare
| // emailValue should only contain alphabets, numbers, hyphen, underscore, dot and @ | ||
| if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/i.test(emailValue)) { return false; } | ||
| // disallow emails that have already been selected or are present in options(dropdown) | ||
| return ![...selectValue, ...options].some(x => x.label.toLowerCase() === emailValue.toLowerCase()); |
There was a problem hiding this comment.
This particular validation should be on component level. This util method should only focus on email text validation.
There was a problem hiding this comment.
I've followed here the same convention which is currently followed for the tags validation
There was a problem hiding this comment.
Right. Tags have a specific use-case. Email is generic, in my opinion.
There was a problem hiding this comment.
I don't find any benefit in moving it to the component level. Because we can reuse this function anywhere after importing it to that file
| /> | ||
| )} | ||
| isMulti | ||
| disabled={!(courseInfo?.data?.course_run_statuses?.includes(REVIEW_BY_INTERNAL) && administrator)} |
There was a problem hiding this comment.
- If I remember the spec, the watchers can be added at any point (but not once scheduled/published). Double-check this.
- We should make this field prominent, either moving to the side pane or moving higher in the course form. If the side pane does not take much effort, that would be preferrable. This particular field does not impact the course content behavior.
There was a problem hiding this comment.
Also, the form remains in an uncleaned state due to the ordering of watcher fields coming from backend. The form field needs to be refreshed. It is already happening for many of the fields in Publisher.
There was a problem hiding this comment.
- I've doubled check this from specs doc
- Ok, I'm moving this field upwards after slug field to make it prominent, adding a side pane and manage field state require some effort, can be done in some followup PR
- Working on it
There was a problem hiding this comment.
Also, the form remains in an uncleaned state due to the ordering of watcher fields coming from backend. The form field needs to be refreshed. It is already happening for many of the fields in Publisher.
Done
AfaqShuaib09
force-pushed
the
afaq/prod_3412
branch
from
9214b7a
to
e5b3ace
Compare
AfaqShuaib09
force-pushed
the
afaq/prod_3412
branch
from
e5b3ace
to
55cfba0
Compare
PROD-3500
This PR adds the watchers field to the course edit form. Its a list of email addresses that will receive notifications when the course run of the course is published or reviewed