diff --git a/reviewboard/accounts/forms/pages.py b/reviewboard/accounts/forms/pages.py index 9444a7f78ff7ca655010962980911d4eae1fc27a..c64bb028fdae7ae612cb41229d3e21d8d57dd3ff 100644 --- a/reviewboard/accounts/forms/pages.py +++ b/reviewboard/accounts/forms/pages.py @@ -1,5 +1,10 @@ +"""My Account configuration pages.""" + +from __future__ import annotations + import logging from collections import OrderedDict +from typing import TYPE_CHECKING from urllib.parse import unquote from django import forms @@ -25,6 +30,9 @@ from reviewboard.site.models import LocalSite from reviewboard.site.urlresolvers import local_site_reverse from reviewboard.themes.ui.registry import ui_theme_registry +if TYPE_CHECKING: + from collections.abc import Mapping + logger = logging.getLogger(__name__) @@ -70,6 +78,11 @@ class AccountSettingsForm(AccountPageForm): syntax_highlighting = forms.BooleanField( label=_('Enable syntax highlighting in the diff viewer'), required=False) + + confirm_ship_it = forms.BooleanField( + label=_('Prompt to confirm when publishing Ship It! reviews'), + required=False) + open_an_issue = forms.BooleanField( label=_('Always open an issue when comment box opens'), required=False) @@ -90,7 +103,24 @@ class AccountSettingsForm(AccountPageForm): label=_('Show desktop notifications'), required=False) - def load(self): + #: Mapping of Profile attribute names to form field names. + #: + #: This is not necessarily comprehensive. Settings that require further + #: processing may not be included. + #: + #: Version Added: + #: 7.1 + _PROFILE_ATTRS_MAP: Mapping[str, str] = { + 'default_use_rich_text': 'default_use_rich_text', + 'open_an_issue': 'open_an_issue', + 'should_confirm_ship_it': 'confirm_ship_it', + 'should_enable_desktop_notifications': 'enable_desktop_notifications', + 'should_send_email': 'should_send_email', + 'should_send_own_updates': 'should_send_own_updates', + 'timezone': 'timezone', + } + + def load(self) -> None: """Load data for the form.""" profile = self.user.get_profile() @@ -98,41 +128,32 @@ class AccountSettingsForm(AccountPageForm): diffviewer_syntax_highlighting = siteconfig.get( 'diffviewer_syntax_highlighting') - self.set_initial({ - 'open_an_issue': profile.open_an_issue, - 'syntax_highlighting': (profile.syntax_highlighting and - diffviewer_syntax_highlighting), - 'timezone': profile.timezone, - 'default_use_rich_text': profile.should_use_rich_text, - 'should_send_email': profile.should_send_email, - 'should_send_own_updates': profile.should_send_own_updates, - 'enable_desktop_notifications': - profile.should_enable_desktop_notifications, - }) + initial = { + field_name: getattr(profile, attr_name) + for attr_name, field_name in self._PROFILE_ATTRS_MAP.items() + } + initial['syntax_highlighting'] = (profile.syntax_highlighting and + diffviewer_syntax_highlighting) + + self.set_initial(initial) if not diffviewer_syntax_highlighting: self.fields['syntax_highlighting'].widget.attrs.update({ 'disabled': True, }) - def save(self): + def save(self) -> None: """Save the form.""" profile = self.user.get_profile() siteconfig = SiteConfiguration.objects.get_current() + cleaned_data = self.cleaned_data if siteconfig.get('diffviewer_syntax_highlighting'): - profile.syntax_highlighting = \ - self.cleaned_data['syntax_highlighting'] - - profile.open_an_issue = self.cleaned_data['open_an_issue'] - profile.default_use_rich_text = \ - self.cleaned_data['default_use_rich_text'] - profile.timezone = self.cleaned_data['timezone'] - profile.should_send_email = self.cleaned_data['should_send_email'] - profile.should_send_own_updates = \ - self.cleaned_data['should_send_own_updates'] - profile.settings['enable_desktop_notifications'] = \ - self.cleaned_data['enable_desktop_notifications'] + profile.syntax_highlighting = cleaned_data['syntax_highlighting'] + + for attr_name, field_name in self._PROFILE_ATTRS_MAP.items(): + setattr(profile, attr_name, cleaned_data[field_name]) + profile.save(update_fields=( 'default_use_rich_text', 'open_an_issue', @@ -149,11 +170,14 @@ class AccountSettingsForm(AccountPageForm): class Meta: fieldsets = ( (_('General Settings'), { - 'fields': ('form_target', - 'timezone', - 'syntax_highlighting', - 'open_an_issue', - 'default_use_rich_text'), + 'fields': ( + 'form_target', + 'timezone', + 'confirm_ship_it', + 'open_an_issue', + 'default_use_rich_text', + 'syntax_highlighting', + ), }), (_('Notifications'), { 'fields': ('should_send_email', diff --git a/reviewboard/accounts/models.py b/reviewboard/accounts/models.py index 20e765cf2b02f3be98cca03d08c75828e055eb3d..8c0cdbe6496fca560d95bbb26f62e6f2eb1ab757 100644 --- a/reviewboard/accounts/models.py +++ b/reviewboard/accounts/models.py @@ -248,6 +248,32 @@ class Profile(models.Model): objects: ClassVar[ProfileManager] = ProfileManager() + @property + def should_confirm_ship_it(self) -> bool: + """Whether to prompt to confirm publishing a Ship It! review. + + Version Added: + 7.1 + """ + return (not self.settings or + self.settings.get('confirm_ship_it', True)) + + @should_confirm_ship_it.setter + def should_confirm_ship_it( + self, + confirm_ship_it: bool, + ) -> None: + """Set whether to prompt to confirm publishing a Ship It! review. + + Version Added: + 7.1 + + Args: + confirm_ship_it (bool): + The new value for the setting. + """ + self.settings['confirm_ship_it'] = confirm_ship_it + @property def should_use_rich_text(self): """Get whether rich text should be used by default for this user. @@ -271,6 +297,10 @@ class Profile(models.Model): explicitly, then that choice will be respected. Otherwise, we enable desktop notifications by default. + Version Changed: + 7.1: + This property can now be changed. + Type: bool: If the user has set whether they wish to receive desktop @@ -280,6 +310,22 @@ class Profile(models.Model): return (not self.settings or self.settings.get('enable_desktop_notifications', True)) + @should_enable_desktop_notifications.setter + def should_enable_desktop_notifications( + self, + enabled: bool, + ) -> None: + """Set whether desktop notifications should be used for this user. + + Version Added: + 7.1 + + Args: + enabled (bool): + The new value for the setting. + """ + self.settings['enable_desktop_notifications'] = enabled + @property def ui_theme_id(self) -> str: """Return the user's preferred UI theme. diff --git a/reviewboard/accounts/templatetags/accounts.py b/reviewboard/accounts/templatetags/accounts.py index 5dd62321ea825fd69de8f4b053db2022560ee7d4..c3c264da9ca2e8b29f191c6c6691d73d08ac68a9 100644 --- a/reviewboard/accounts/templatetags/accounts.py +++ b/reviewboard/accounts/templatetags/accounts.py @@ -121,6 +121,7 @@ def js_user_session_info(context): use_rich_text = profile.should_use_rich_text info.update({ 'commentsOpenAnIssue': profile.open_an_issue, + 'confirmShipIt': profile.should_confirm_ship_it, 'enableDesktopNotifications': profile.should_enable_desktop_notifications, }) diff --git a/reviewboard/accounts/tests/test_template_tags.py b/reviewboard/accounts/tests/test_template_tags.py index 16b98a9840ba6f56760a6aedf143f34f280c6448..79716a1a08390d87b916e62ee4b64b21a7484116 100644 --- a/reviewboard/accounts/tests/test_template_tags.py +++ b/reviewboard/accounts/tests/test_template_tags.py @@ -80,6 +80,7 @@ class JSUserSessionInfoTests(TestCase): }, }, 'commentsOpenAnIssue': True, + 'confirmShipIt': True, 'defaultUseRichText': True, 'enableDesktopNotifications': True, 'fullName': 'Test User', @@ -146,6 +147,7 @@ class JSUserSessionInfoTests(TestCase): }, }, 'commentsOpenAnIssue': True, + 'confirmShipIt': True, 'defaultUseRichText': True, 'enableDesktopNotifications': True, 'fullName': 'Test User', @@ -205,6 +207,7 @@ class JSUserSessionInfoTests(TestCase): }, }, 'commentsOpenAnIssue': True, + 'confirmShipIt': True, 'defaultUseRichText': False, 'enableDesktopNotifications': True, 'fullName': 'Test User', @@ -242,6 +245,7 @@ class JSUserSessionInfoTests(TestCase): 'avatarURLs': {}, 'avatarHTML': {}, 'commentsOpenAnIssue': True, + 'confirmShipIt': True, 'defaultUseRichText': True, 'enableDesktopNotifications': True, 'fullName': 'Test User', diff --git a/reviewboard/static/rb/js/common/models/userSessionModel.ts b/reviewboard/static/rb/js/common/models/userSessionModel.ts index a091ccc5f0c1133f605240e4dd668625e3f8cbb0..67942337806c93a63cb11d9b7471818e51a9d494 100644 --- a/reviewboard/static/rb/js/common/models/userSessionModel.ts +++ b/reviewboard/static/rb/js/common/models/userSessionModel.ts @@ -3,6 +3,7 @@ */ import { type ModelAttributes, + type Result, BaseModel, spina, } from '@beanbag/spina'; @@ -266,6 +267,14 @@ interface UserSessionAttrs extends ModelAttributes { /** Whether to open an issue by default */ commentsOpenAnIssue: boolean; + /** + * Whether to prompt to confirm publishing a Ship It! review. + * + * Version Added: + * 7.1 + */ + confirmShipIt: boolean; + /** Whether to use rich text by default. */ defaultUseRichText: boolean; @@ -320,6 +329,17 @@ interface UserSessionAttrs extends ModelAttributes { } +/** + * A map of UserSession attributes to stored settings names. + * + * Version Added: + * 7.1 + */ +const _storeSettingsMap: Record = { + 'confirmShipIt': 'confirm_ship_it', +}; + + /** * Manages the user's active session. * @@ -367,6 +387,7 @@ export class UserSession extends BaseModel { avatarHTML: {}, avatarURLs: {}, commentsOpenAnIssue: true, + confirmShipIt: true, defaultUseRichText: false, diffsShowExtraWhitespace: false, enableDesktopNotifications: false, @@ -477,6 +498,48 @@ export class UserSession extends BaseModel { return urls[size] || ''; } + /** + * Store a setting for the user. + * + * Only a pre-defined list of settings are supported. + * + * This should be considered internal to the Review Board codebase. + * + * Version Added: + * 7.1 + * + * Args: + * setting (Array of string): + * The attributes for the settings to store on the server. + * + * Returns: + * Promise: + * The promise for the operation. + */ + storeSettings( + settings: (keyof typeof _storeSettingsMap)[], + ): Promise { + return new Promise((success, error) => { + RB.apiCall({ + data: { + 'settings:json': JSON.stringify(Object.fromEntries( + settings.map(( + setting: string, + ) => [ + _storeSettingsMap[setting], + this.get(setting), + ]) + )), + }, + path: '/session/', + type: 'PUT', + + error: () => error(), + success: () => success(), + }); + }); + } + /** * Bind a cookie to an attribute. * diff --git a/reviewboard/static/rb/js/reviews/views/reviewablePageView.ts b/reviewboard/static/rb/js/reviews/views/reviewablePageView.ts index 6ad3bd61ab702171773daaff419b7f73d0cd92ac..f785dd36b5cab9ecc8196f55e840e998bc694ed5 100644 --- a/reviewboard/static/rb/js/reviews/views/reviewablePageView.ts +++ b/reviewboard/static/rb/js/reviews/views/reviewablePageView.ts @@ -2,7 +2,10 @@ * A page managing reviewable content for a review request. */ import { + type DialogView, + DialogActionType, craft, + paint, renderInto, } from '@beanbag/ink'; import { @@ -593,20 +596,112 @@ export class ReviewablePageView< * e (JQuery.ClickEvent, optional): * The event which triggered the action, if available. */ - async shipIt(e?: JQuery.ClickEvent) { + shipIt(e?: JQuery.ClickEvent) { if (e) { e.preventDefault(); e.stopPropagation(); } - if (confirm(_`Are you sure you want to post this review?`)) { - await this.model.markShipIt(); - - const reviewRequest = this.model.get('reviewRequest'); - RB.navigateTo(reviewRequest.get('reviewURL')); + const session = UserSession.instance; + + if (session.get('confirmShipIt')) { + const onConfirm = async () => { + if (doNotAskEl.checked) { + session.set('confirmShipIt', false); + await session.storeSettings(['confirmShipIt']); + } + + await this.#postShipItReview(); + }; + + const cid = this.cid; + const doNotAskEl = paint` + + `; + + /* + * Work around bad default layout for checkboxes and labels. + * + * Ideally this would be in CSS, but this is a stopgap until we + * have a more formal component for this kind of dialog or for + * form controls. + */ + const doNotAskStyle = { + 'align-items': 'center', + 'display': 'flex', + 'gap': '0.5ch', + 'margin': '2em 0 0 0', + }; + + const dialogView = craft` + + +

+ ${_` + This review will tell the author that you approve of their + review request. + `} +

+

+ ${_`Tip:`} + ${_` + You can revoke this Ship It! or publish new reviews + after this is published. + `} +

+ + + + onConfirm()}> + ${_`Post the Review`} + + + ${_`Cancel`} + + + + `; + + dialogView.open(); + } else { + this.#postShipItReview(); } + } - return false; + /** + * Post a Ship-It! review. + * + * After posting, this will navigate to the review request page. + * + * Version Added: + * 7.1 + * + * Returns: + * Promise: + * The promise for the operation. + */ + async #postShipItReview() { + const model = this.model; + + await model.markShipIt(); + + const reviewRequest = model.get('reviewRequest'); + RB.navigateTo(reviewRequest.get('reviewURL')); } /** diff --git a/reviewboard/static/rb/js/reviews/views/tests/reviewablePageViewTests.ts b/reviewboard/static/rb/js/reviews/views/tests/reviewablePageViewTests.ts index 5b37d0dd22f6ff10129e3ffded154cacb9a1ee17..0585c7515ab95382f25eb7dabc3b71c8cac84e00 100644 --- a/reviewboard/static/rb/js/reviews/views/tests/reviewablePageViewTests.ts +++ b/reviewboard/static/rb/js/reviews/views/tests/reviewablePageViewTests.ts @@ -12,6 +12,7 @@ import { import { EnabledFeatures, ReviewRequest, + UserSession, } from 'reviewboard/common'; import { ReviewablePage, @@ -142,53 +143,104 @@ suite('rb/pages/views/ReviewablePageView', function() { expect(options.reviewRequestEditor).toBe(page.reviewRequestEditor); }); - describe('Ship It', function() { + describe('Ship It', () => { let pendingReview; + let cid: string; + let userSession: UserSession; + + beforeEach(() => { + spyOn(page, 'markShipIt').and.resolveTo(); - beforeEach(function() { pendingReview = page.get('pendingReview'); + cid = pageView.cid; + userSession = UserSession.instance; }); - it('Confirmed', async function() { - if (EnabledFeatures.unifiedBanner) { - pending(); + it('Confirmed', done => { + spyOn(pendingReview, 'save').and.resolveTo(); + spyOn(pendingReview, 'publish').and.callThrough(); + spyOn(userSession, 'storeSettings').and.callThrough(); + RB.navigateTo.and.callFake(() => { + expect(userSession.get('confirmShipIt')).toBeTrue(); + expect(userSession.storeSettings).not.toHaveBeenCalled(); + expect(page.markShipIt).toHaveBeenCalled(); + + done(); + }); + + pageView.shipIt(); - return; - } + const dialogEl = document.getElementById( + `confirm-ship-it-dialog-${cid}` + ) as HTMLDialogElement; - spyOn(window, 'confirm').and.returnValue(true); + expect(dialogEl).not.toBeNull(); + + $(`#confirm-ship-it-button-${cid}`).click(); + }); + + it('Confirmed with Do Not Ask Again', done => { spyOn(pendingReview, 'save').and.resolveTo(); spyOn(pendingReview, 'publish').and.callThrough(); + spyOn(userSession, 'storeSettings').and.callThrough(); + RB.navigateTo.and.callFake(() => { + expect(userSession.get('confirmShipIt')).toBeFalse(); + expect(userSession.storeSettings).toHaveBeenCalledWith([ + 'confirmShipIt', + ]); + expect(page.markShipIt).toHaveBeenCalled(); + + done(); + }); + + pageView.shipIt(); + + const dialogEl = document.getElementById( + `confirm-ship-it-dialog-${cid}` + ) as HTMLDialogElement; + + expect(dialogEl).not.toBeNull(); + + const checkboxEl = document.getElementById( + `confirm-ship-it-do-not-ask-${cid}` + ) as HTMLInputElement; + checkboxEl.checked = true; + + $(`#confirm-ship-it-button-${cid}`).click(); + }); + + it('Without confirmation dialog', done => { + userSession.set('confirmShipIt', false); + + spyOn(pendingReview, 'save').and.resolveTo(); + spyOn(pendingReview, 'publish').and.callThrough(); + RB.navigateTo.and.callFake(() => { + const dialogEl = document.getElementById( + `confirm-ship-it-dialog-${cid}` + ) as HTMLDialogElement; + + expect(dialogEl).toBeNull(); + + done(); + }); - if (!EnabledFeatures.unifiedBanner) { - spyOn(pageView.draftReviewBanner, 'hideAndReload') - .and.callFake(() => { - expect(window.confirm).toHaveBeenCalled(); - expect(pendingReview.ready).toHaveBeenCalled(); - expect(pendingReview.publish).toHaveBeenCalled(); - expect(pendingReview.save).toHaveBeenCalled(); - expect(pendingReview.get('shipIt')).toBe(true); - expect(pendingReview.get('bodyTop')) - .toBe('Ship It!'); - }); - } - - await pageView.shipIt(); + pageView.shipIt(); }); - it('Canceled', async function() { - if (EnabledFeatures.unifiedBanner) { - pending(); + it('Canceled', () => { + pageView.shipIt(); - return; - } + const dialogEl = document.getElementById( + `confirm-ship-it-dialog-${cid}` + ) as HTMLDialogElement; - spyOn(window, 'confirm').and.returnValue(false); + expect(dialogEl).not.toBeNull(); - await pageView.shipIt(); + $(`#cancel-ship-it-button-${cid}`).click(); - expect(window.confirm).toHaveBeenCalled(); - expect(pendingReview.ready).not.toHaveBeenCalled(); + expect(page.markShipIt).not.toHaveBeenCalled(); + expect(RB.navigateTo).not.toHaveBeenCalled(); + expect(dialogEl.open).toBeFalse(); }); }); }); diff --git a/reviewboard/templates/js/tests_base.html b/reviewboard/templates/js/tests_base.html index 4d7166c3ff02b5c7e1d01ff9f1296f64d0e8043f..13b866f6808723be7a7b79139d4843a515b37d23 100644 --- a/reviewboard/templates/js/tests_base.html +++ b/reviewboard/templates/js/tests_base.html @@ -84,6 +84,7 @@ RB.UserSession.create({ authenticated: true, commentsOpenAnIssue: true, + confirmShipIt: true, username: 'testuser', userPageURL: '{{SITE_ROOT}}users/test/' }); diff --git a/reviewboard/webapi/resources/session.py b/reviewboard/webapi/resources/session.py index 683861f5356281348f433abe6ccf0a0d1f3c5504..30d146249452a99f6787c3b7aee59b02bf2e99f8 100644 --- a/reviewboard/webapi/resources/session.py +++ b/reviewboard/webapi/resources/session.py @@ -1,11 +1,29 @@ +"""API resource for user session management.""" + +from __future__ import annotations + +import json +from typing import TYPE_CHECKING + from django.contrib.auth import logout -from djblets.webapi.decorators import webapi_login_required +from django.contrib.auth.models import User +from django.utils.translation import gettext as _ +from djblets.util.json_utils import json_merge_patch +from djblets.webapi.decorators import (webapi_login_required, + webapi_request_fields) +from djblets.webapi.errors import INVALID_FORM_DATA from djblets.webapi.resources.registry import get_resource_for_object from reviewboard.webapi.base import WebAPIResource from reviewboard.webapi.decorators import (webapi_check_login_required, webapi_check_local_site) +if TYPE_CHECKING: + from django.http import HttpRequest + + from djblets.webapi.resources.base import WebAPIResourceHandlerResult + from djblets.webapi.responses import WebAPIResponsePayload + class SessionResource(WebAPIResource): """Information on the active user's session. @@ -15,33 +33,88 @@ class SessionResource(WebAPIResource): own resource, making it easy to figure out the user's information and any useful related resources. """ + name = 'session' singleton = True - allowed_methods = ('GET', 'DELETE') - - @webapi_check_local_site - @webapi_check_login_required - def get(self, request, *args, **kwargs): - """Returns information on the client's session. - - This currently just contains information on the currently logged-in - user (if any). + allowed_methods = ('GET', 'PUT', 'DELETE') + + #: Settings paths that can be modified in a PUT. + #: + #: Version Added: + #: 7.1 + _MUTABLE_PROFILE_SETTING_PATHS: set[tuple[str, ...]] = { + ('confirm_ship_it',), + } + + def serialize_object( + self, + obj: None, + request: (HttpRequest | None) = None, + *args, + **kwargs, + ) -> WebAPIResponsePayload: + """Serialize the session resource. + + This specially serializes some state and links for the session + based on the login state. + + Args: + obj (object): + The object to serialize. + + This will always be ``None`` for this resource. + + request (django.http.HttpRequest, optional): + The HTTP request from the client. + + *args (tuple): + Positional arguments passed to the view. + + **kwargs (dict): + Keyword arguments passed to the view. + + Returns: + djblets.webapi.responses.WebAPIResponsePayload: + The serialized payload. """ - expanded_resources = request.GET.get('expand', '').split(',') + assert request is not None + + expanded_resources = ( + request.GET.get('expand', '') or + request.POST.get('expand', '') + ).split(',') - authenticated = request.user.is_authenticated + user = request.user + authenticated = user.is_authenticated - data = { + data: WebAPIResponsePayload = { 'authenticated': authenticated, 'links': self.get_links(request=request, *args, **kwargs), } if authenticated and 'user' in expanded_resources: - data['user'] = request.user + data['user'] = user del data['links']['user'] + return data + + @webapi_check_local_site + @webapi_check_login_required + def get( + self, + request: HttpRequest, + *args, + **kwargs, + ) -> WebAPIResourceHandlerResult: + """Returns information on the client's session. + + This currently just contains information on the currently logged-in + user (if any). + """ return 200, { - self.name: data, + self.name: self.serialize_object(obj=None, + request=request, + *args, **kwargs), } @webapi_check_local_site @@ -58,6 +131,85 @@ class SessionResource(WebAPIResource): return 204, {} + @webapi_login_required + @webapi_check_local_site + @webapi_request_fields( + optional={ + 'settings:json': { + 'type': str, + 'description': ( + 'A JSON Merge Patch of settings change to make. These ' + 'will be persisted across sessions. This is considered ' + 'internal API and is subject to change.' + ), + }, + }, + ) + def update( + self, + request: HttpRequest, + *args, + **kwargs, + ) -> WebAPIResourceHandlerResult: + """Update information about the session. + + This is only used internally to manage Review Board session state. + This operation is not considered public API. + + Version Added: + 7.1 + """ + settings_json = request.POST.get('settings:json') + + if settings_json: + try: + patch = json.loads(settings_json) + except ValueError as e: + return INVALID_FORM_DATA, { + 'fields': { + 'settings': [ + # Use %-based formatting to share translations + # with ImportExtraDataError.error_payload. + _('Could not parse JSON data: %s') % e, + ], + }, + } + + user = request.user + assert isinstance(user, User) + + profile = user.get_profile(create_if_missing=True) + + MUTABLE_SETTING_PATHS = self._MUTABLE_PROFILE_SETTING_PATHS + + new_settings = json_merge_patch( + profile.settings, + patch, + can_write_key_func=lambda *, path, **kwargs: + path in MUTABLE_SETTING_PATHS) + + # Save extra_data only if it remains a dictionary, so callers + # can't replace the entire contents. + if not isinstance(new_settings, dict): + return INVALID_FORM_DATA, { + 'fields': { + 'settings': [ + _('settings:json cannot replace the settings ' + 'with a non-dictionary type'), + ], + }, + } + + profile.settings.clear() + profile.settings.update(new_settings) + profile.save(update_fields=('settings',)) + + return 200, { + self.name: self.serialize_object(obj=None, + request=request, + *args, **kwargs), + } + def get_related_links(self, obj=None, request=None, *args, **kwargs): links = {} diff --git a/reviewboard/webapi/tests/test_session.py b/reviewboard/webapi/tests/test_session.py index 6747433e433251abef44851f1a17b8a90adce5b2..24378f6237adfcce142ada03928e0a71afc5ea81 100644 --- a/reviewboard/webapi/tests/test_session.py +++ b/reviewboard/webapi/tests/test_session.py @@ -1,3 +1,10 @@ +"""Unit tests for reviewboard.webapi.resources.session.""" + +from __future__ import annotations + +import json +from typing import TYPE_CHECKING + from django.http import SimpleCookie from djblets.webapi.errors import NOT_LOGGED_IN from djblets.webapi.testing.decorators import webapi_test_template @@ -8,9 +15,18 @@ from reviewboard.webapi.tests.mimetypes import session_mimetype from reviewboard.webapi.tests.mixins import BasicTestsMetaclass from reviewboard.webapi.tests.urls import get_session_url +if TYPE_CHECKING: + from typing import Any + + from django.contrib.auth.models import User + from djblets.util.typing import JSONDict + + from reviewboard.webapi.tests.mixins import BasicPutTestSetupState + class ResourceTests(BaseWebAPITestCase, metaclass=BasicTestsMetaclass): """Testing the SessionResource APIs.""" + fixtures = ['test_users'] sample_api_url = 'session/' resource = resources.session @@ -40,11 +56,24 @@ class ResourceTests(BaseWebAPITestCase, metaclass=BasicTestsMetaclass): def test_get_with_anonymous_user(self): """Testing the GET session/ API with anonymous user""" self.client.logout() + rsp = self.api_get(get_session_url(), expected_mimetype=session_mimetype) - self.assertEqual(rsp['stat'], 'ok') - self.assertIn('session', rsp) - self.assertFalse(rsp['session']['authenticated']) + + self.assertEqual( + rsp, + { + 'session': { + 'authenticated': False, + 'links': { + 'self': { + 'href': 'http://testserver/api/session/', + 'method': 'GET', + }, + }, + }, + 'stat': 'ok', + }) # # HTTP DELETE test @@ -68,5 +97,134 @@ class ResourceTests(BaseWebAPITestCase, metaclass=BasicTestsMetaclass): self.client.cookies = SimpleCookie() rsp = self.api_delete(url, expected_status=401) - self.assertEqual(rsp['stat'], 'fail') - self.assertEqual(rsp['err']['code'], NOT_LOGGED_IN.code) + + self.assertEqual( + rsp, + { + 'err': { + 'code': NOT_LOGGED_IN.code, + 'msg': NOT_LOGGED_IN.msg, + 'type': NOT_LOGGED_IN.error_type, + }, + 'stat': 'fail', + }) + + # + # HTTP PUT tests + # + + # This test does not apply, so remove it. + test_put_not_owner = None + + def populate_put_test_objects( + self, + *, + setup_state: BasicPutTestSetupState, + create_valid_request_data: bool, + **kwargs, + ) -> None: + """Populate objects for a PUT test. + + Version Added: + 7.1 + + Args: + setup_state (reviewboard.webapi.tests.mixins. + BasicPutTestSetupState): + The setup state for the test. + + create_valid_request_data (bool): + Whether ``request_data`` in ``setup_state`` should provide + valid data for a PUT test, given the populated objects. + + **kwargs (dict): + Additional keyword arguments for future expansion. + """ + setup_state.update({ + 'item': None, + 'mimetype': session_mimetype, + 'request_data': {}, + 'url': get_session_url( + local_site_name=setup_state['local_site_name']), + }) + + def check_put_result( + self, + user: User, + item_rsp: JSONDict, + item: Any, + *args, + ) -> None: + """Check the results of an HTTP PUT. + + Version Added: + 7.1 + + Args: + user (django.contrib.auth.models.User): + The user performing the requesdt. + + item_rsp (dict): + The item payload from the response. + + item (object): + The item to compare to. + + *args (tuple): + Positional arguments provided by + :py:meth:`setup_basic_put_test`. + + Raises: + AssertionError: + One of the checks failed. + """ + self.compare_item(item_rsp, user) + + @webapi_test_template + def test_put_with_settings_json(self) -> None: + """Testing the PUT API with settings:json""" + user = self._login_user() + + profile = user.get_profile() + profile.settings['some_setting'] = '123' + profile.save(update_fields=('settings',)) + + rsp = self.api_put( + get_session_url(), + { + 'settings:json': json.dumps({ + 'bad_option': '123', + 'confirm_ship_it': False, + }) + }, + expected_mimetype=session_mimetype) + + self.assertEqual( + rsp, + { + 'session': { + 'authenticated': True, + 'links': { + 'delete': { + 'href': 'http://testserver/api/session/', + 'method': 'DELETE', + }, + 'self': { + 'href': 'http://testserver/api/session/', + 'method': 'GET', + }, + 'user': { + 'href': 'http://testserver/api/users/grumpy/', + 'method': 'GET', + 'title': 'grumpy', + }, + }, + }, + 'stat': 'ok', + }) + + profile.refresh_from_db() + self.assertEqual(profile.settings, { + 'some_setting': '123', + 'confirm_ship_it': False, + })