Repository: mwclient/mwclient

Issue:
When you create a page, mwclient still thinks it doesn't exist
Another one I found writing integration tests. To reproduce:
```
pg = site.pages["NonExistentPage"]
pg.exists
pg.edit("Hi I'm a new page", "create new page")
pg.exists
```
`pg.exists` will be `False` both times, when obviously it should be `True` the second time (assuming the edit worked). You have to re-initialize the page after editing to get the right result:
```
pg = site.pages["NonExistentPage"]
pg.exists
pg.edit("Hi I'm a new page", "create new page")
pg = site.pages["NonExistentPage"]
pg.exists
```
The easy-looking way to fix this would be just to add a `self.exists = True` call to `Page._edit` at an appropriate point where we're really sure the edit actually *did something*, probably in the `if 'newtimestamp' in result['edit'].keys():` block. I'm not absolutely 100% sure that's safe, though. The probably-better way to fix it would be to turn `exists` into a property and have it depend on the page info...we might want to have a separate property for the page info, in fact. `Page.__init__` stashes the info it gets as `self._info` and it's never updated, which looks kind of...wrong.

Thoughts? @marcfrederick @RheingoldRiver 

Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/70167630/badge)](https://coveralls.io/builds/70167630)

coverage: 74.036% (+0.4%) from 73.657%
when pulling **dbb393e6e3b4fec9eced7bdc5a8de7b1c7ec6684 on marcfrederick:feature/page-edit-update-state**
into **722fc47ddaa8b9a041ce4bc441125b50a65aa199 on mwclient:master**.

AdamWill: So compared to my patch, this handles the `edit` path much more extensively (nice job), but I also figured it made sense to update status in `move`:

        result = self.site.post('move', ('from', self.name), to=new_title,
                                token=self.get_token('move'), reason=reason, **data)
        if 'redirectcreated' not in result:
            # no redirect, so this page no longer exists
            self.exists = False
        return result['move']

and `delete`:

        result = self.site.post('delete', title=self.name,
                                token=self.get_token('delete'),
                                reason=reason, **data)
        # if the POST did not raise an exception, we don't exist
        self.exists = False
        return result['delete']

wdyt about those?
marcfrederick: Yeah, makes sense. I’ve updated the PR with those changes and added some tests. I’ve also changed `Page.move` to set `Page.exists = False` when no redirect is created.
AdamWill: @RheingoldRiver wdyt? I think you're entirely correct that the pages-as-persistent-instances design is inherently a bit wrong-headed and thus this can *always* potentially be a lie, but given we're stuck maintaining a mature library with said wrong-headed design, making it *less likely* to be a lie is overall still a win.
RheingoldRiver: I think this should be okay, it's a pretty conservative change, and I can't imagine any sane situation in which it would break someone's code. LGTM

Patch:
diff --git a/mwclient/listing.py b/mwclient/listing.py
index ef5780f..546d8c4 100644
--- a/mwclient/listing.py
+++ b/mwclient/listing.py
@@ -1,5 +1,5 @@
 from typing import (  # noqa: F401
-    Optional, Tuple, Any, Union, Iterator, Mapping, Iterable, Type
+    Optional, Tuple, Any, Union, Iterator, Mapping, Iterable, Type, Dict
 )
 
 import mwclient.image
@@ -237,7 +237,7 @@ class Category(mwclient.page.Page, GeneratorList):
         namespace: Optional[Namespace] = None
     ) -> None:
         mwclient.page.Page.__init__(self, site, name, info)
-        kwargs = {}
+        kwargs = {}  # type: Dict[str, Any]
         kwargs['gcmtitle'] = self.name
         if namespace:
             kwargs['gcmnamespace'] = namespace
diff --git a/mwclient/page.py b/mwclient/page.py
index edac3e9..4dfee70 100644
--- a/mwclient/page.py
+++ b/mwclient/page.py
@@ -1,6 +1,6 @@
 import time
 from typing import (  # noqa: F401
-    Optional, Mapping, Any, cast, Dict, Union, Tuple, Iterable, List
+    Optional, Mapping, Any, cast, Dict, Union, Tuple, Iterable, List, NoReturn
 )
 
 import mwclient.errors
@@ -73,15 +73,7 @@ class Page:
             raise mwclient.errors.InvalidPageTitle(info.get('invalidreason'))
 
         self.namespace = info.get('ns', 0)
-        self.name = info.get('title', '')
-        if self.namespace:
-            self.page_title = self.strip_namespace(self.name)
-        else:
-            self.page_title = self.name
-
-        self.base_title = self.page_title.split('/')[0]
-        self.base_name = self.name.split('/')[0]
-
+        self.name = info.get('title', '')  # type: str
         self.touched = parse_timestamp(info.get('touched'))
         self.revision = info.get('lastrevid', 0)
         self.exists = 'missing' not in info
@@ -100,6 +92,21 @@ class Page:
         self.last_rev_time = None  # type: Optional[time.struct_time]
         self.edit_time = None  # type: Optional[time.struct_time]
 
+    @property
+    def page_title(self) -> str:
+        if self.namespace:
+            return self.strip_namespace(self.name)
+        else:
+            return self.name
+
+    @property
+    def base_title(self) -> str:
+        return self.page_title.split('/')[0]
+
+    @property
+    def base_name(self) -> str:
+        return self.name.split('/')[0]
+
     def redirects_to(self) -> Optional['Page']:
         """ Get the redirect target page, or None if the page is not a redirect."""
         info = self.site.get('query', prop='pageprops', titles=self.name, redirects='')
@@ -286,7 +293,7 @@ class Page:
         if self.site.force_login:
             data['assert'] = 'user'
 
-        def do_edit() -> Any:
+        def do_edit() -> Dict[str, Any]:
             result = self.site.post('edit', title=self.name, summary=summary,
                                     token=self.get_token('edit'),
                                     **data)
@@ -307,9 +314,16 @@ class Page:
             else:
                 self.handle_edit_error(e, summary)
 
+        self.exists = True
+        self.name = result['edit'].get('title', self.name)
+        self.pageid = result['edit'].get('pageid', self.pageid)
+        self.revision = result['edit'].get('newrevid', self.revision)
+        self.contentmodel = result['edit'].get('contentmodel', self.contentmodel)
         # 'newtimestamp' is not included if no change was made
         if 'newtimestamp' in result['edit'].keys():
-            self.last_rev_time = parse_timestamp(result['edit'].get('newtimestamp'))
+            new_timestamp = parse_timestamp(result['edit'].get('newtimestamp'))
+            self.last_rev_time = new_timestamp
+            self.touched = new_timestamp
 
         # Workaround for https://phabricator.wikimedia.org/T211233
         for cookie in self.site.connection.cookies:
@@ -321,7 +335,7 @@ class Page:
         self._textcache = {}
         return result['edit']
 
-    def handle_edit_error(self, e: 'mwclient.errors.APIError', summary: str) -> None:
+    def handle_edit_error(self, e: 'mwclient.errors.APIError', summary: str) -> NoReturn:
         if e.code == 'editconflict':
             raise mwclient.errors.EditError(self, summary, e.info)
         elif e.code in {'protectedtitle', 'cantcreate', 'cantcreate-anon',
@@ -376,8 +390,15 @@ class Page:
             data['movesubpages'] = '1'
         if ignore_warnings:
             data['ignorewarnings'] = '1'
+
         result = self.site.post('move', ('from', self.name), to=new_title,
                                 token=self.get_token('move'), reason=reason, **data)
+
+        if 'redirectcreated' in result['move']:
+            self.redirect = True
+        else:
+            self.exists = False
+
         return result['move']
 
     def delete(
@@ -404,9 +425,12 @@ class Page:
             data['unwatch'] = '1'
         if oldimage:
             data['oldimage'] = oldimage
+
         result = self.site.post('delete', title=self.name,
                                 token=self.get_token('delete'),
                                 reason=reason, **data)
+
+        self.exists = False
         return result['delete']
 
     def purge(self) -> None:


Test patch:
diff --git a/test/test_page.py b/test/test_page.py
index 69149ae..a6ad61a 100644
--- a/test/test_page.py
+++ b/test/test_page.py
@@ -1,6 +1,9 @@
+import time
 import unittest
 import unittest.mock as mock
+
 import pytest
+
 import mwclient
 from mwclient.errors import APIError, AssertUserFailedError, ProtectedPageError, \
     InvalidPageTitle
@@ -211,6 +214,123 @@ class TestPage(unittest.TestCase):
         with pytest.raises(mwclient.errors.EditError):
             page.edit('Some text')
 
+    @mock.patch('mwclient.client.Site')
+    def test_edit(self, mock_site):
+        mock_site.blocked = False
+        mock_site.rights = ['read', 'edit']
+        mock_site.get.return_value = {'query': {'pages': {
+            '-1': {'ns': 1, 'title': 'Talk:Some page/Archive 1', 'missing': ''}
+        }}}
+        page = Page(mock_site, 'Talk:Some page/Archive 1')
+
+        mock_site.post.return_value = {
+            'edit': {'result': 'Success', 'pageid': 1234,
+                     'title': 'Talk:Some page/Archive 1', 'contentmodel': 'wikitext',
+                     'oldrevid': 123456, 'newrevid': 123457,
+                     'newtimestamp': '2024-10-02T12:34:07Z'}
+
+        }
+        page.edit('Some text')
+
+        mock_site.post.assert_called_once()
+        assert page.exists, 'Page should exist after edit'
+        assert page.pageid == 1234
+        assert page.name == 'Talk:Some page/Archive 1'
+        assert page.page_title == 'Some page/Archive 1'
+        assert page.base_title == 'Some page'
+        assert page.base_name == 'Talk:Some page'
+        assert page.contentmodel == 'wikitext'
+        assert page.revision == 123457
+        assert page.last_rev_time == time.struct_time(
+            (2024, 10, 2, 12, 34, 7, 2, 276, -1)
+        )
+        assert page.touched == time.struct_time(
+            (2024, 10, 2, 12, 34, 7, 2, 276, -1)
+        )
+
+    @mock.patch('mwclient.client.Site')
+    def test_delete(self, mock_site):
+        mock_site.rights = ['read', 'delete']
+        page_title = 'Some page'
+        page = Page(mock_site, page_title, info={
+            'contentmodel': 'wikitext',
+            'counter': '',
+            'lastrevid': 13355471,
+            'length': 58487,
+            'ns': 0,
+            'pageid': 728,
+            'pagelanguage': 'nb',
+            'protection': [],
+            'title': page_title,
+            'touched': '2014-09-14T21:11:52Z'
+        })
+
+        reason = 'Some reason'
+        mock_site.post.return_value = {
+            'delete': {'title': page_title, 'reason': reason, 'logid': 1234}
+        }
+        page.delete(reason)
+
+        mock_site.post.assert_called_once_with(
+            'delete', title=page_title, reason=reason, token=mock.ANY
+        )
+        assert not page.exists, 'Page should not exist after delete'
+
+    @mock.patch('mwclient.client.Site')
+    def test_move(self, mock_site):
+        mock_site.rights = ['read', 'move']
+        page_title = 'Some page'
+        page = Page(mock_site, page_title, info={
+            'contentmodel': 'wikitext',
+            'counter': '',
+            'lastrevid': 13355471,
+            'length': 58487,
+            'ns': 0,
+            'pageid': 728,
+            'pagelanguage': 'nb',
+            'protection': [],
+            'title': page_title,
+            'touched': '2014-09-14T21:11:52Z'
+        })
+
+        new_title = 'Some new page'
+        reason = 'Some reason'
+        mock_site.post.return_value = {
+            'move': {'from': page_title, 'to': new_title, 'reason': reason,
+                     'redirectcreated': ''}
+        }
+        page.move(new_title, reason)
+
+        assert page.exists, 'Page should still exist after move'
+        assert page.redirect, 'Page should be a redirect after move'
+
+    @mock.patch('mwclient.client.Site')
+    def test_move_no_redirect(self, mock_site):
+        mock_site.rights = ['read', 'move']
+        page_title = 'Some page'
+        page = Page(mock_site, page_title, info={
+            'contentmodel': 'wikitext',
+            'counter': '',
+            'lastrevid': 13355471,
+            'length': 58487,
+            'ns': 0,
+            'pageid': 728,
+            'pagelanguage': 'nb',
+            'protection': [],
+            'title': page_title,
+            'touched': '2014-09-14T21:11:52Z'
+        })
+
+        new_title = 'Some new page'
+        reason = 'Some reason'
+        mock_site.post.return_value = {
+            'move': {'from': page_title, 'to': new_title, 'reason': reason}
+        }
+        page.move(new_title, reason, no_redirect=True)
+
+        assert not page.exists, 'Page should not exist after move'
+        assert not page.redirect, 'Page should not be a redirect after move'
+
 
 class TestPageApiArgs(unittest.TestCase):